feat(userspace): web app-runtime foundation (#89 P3a)#970
Conversation
Qodo reviews are paused for this user.Troubleshooting steps vary by plan Learn more → On a Teams plan? Using GitHub Enterprise Server, GitLab Self-Managed, or Bitbucket Data Center? |
📝 WalkthroughWalkthroughIntroduces a complete sandboxed ChangesUserspace Apps Runtime
Sequence Diagram(s)sequenceDiagram
participant WebApp as Sandboxed iframe (web app)
participant SDK as taos-app-sdk.js
participant SandboxedAppWindow as SandboxedAppWindow (React)
participant BrokerRoute as POST /api/userspace-apps/{app_id}/broker
participant handle_capability as handle_capability
WebApp->>SDK: window.taos.kv.get("mykey")
SDK->>SandboxedAppWindow: postMessage { taosApp, id, capability: "app.kv.get", args }
SandboxedAppWindow->>BrokerRoute: fetch POST with { capability, args }, credentials: include
BrokerRoute->>handle_capability: capability, args, granted, data_store, app_dir, services
handle_capability-->>BrokerRoute: { result: value }
BrokerRoute-->>SandboxedAppWindow: 200 JSON { result: value }
SandboxedAppWindow->>SDK: iframe.contentWindow.postMessage { taosAppReply: id, result: value }
SDK-->>WebApp: resolved Promise(value)
Estimated code review effort🎯 5 (Critical) | ⏱️ ~120 minutes Poem
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![gitar-bot[bot]](https://avatars.githubusercontent.com/in/827041?s=60&v=4){kind=small}
| try: | ||
| manifest = extract_package(data, apps_root=_apps_root(request)) | ||
| except PackageError as exc: | ||
| return JSONResponse({"error": str(exc)}, status_code=400) | ||
| existing = await store.get(manifest["id"]) | ||
| new_perms = [ | ||
| p for p in manifest["permissions"] | ||
| if existing and p not in existing["permissions_granted"] | ||
| ] | ||
| await store.install( | ||
| app_id=manifest["id"], name=manifest["name"], version=manifest["version"], | ||
| app_type=manifest["app_type"], entry=manifest["entry"], icon=manifest["icon"], | ||
| permissions_requested=manifest["permissions"], | ||
| ) | ||
| deploy_info: dict = {} |
There was a problem hiding this comment.
⚠️ Bug: Container install persists app + files then returns 500
In install_app, the order of operations is: extract_package(...) writes the bundle to apps_root/{id}/, then store.install(...) persists the app row, and only afterwards does the code check if manifest['app_type'] == 'container' and raise NotImplementedError("container packages: web-only for now"). Because the exception is raised after both the disk extraction and the DB insert, a container .taosapp install:
- Returns an unhandled 500 to the caller (NotImplementedError is not converted to a 4xx), and
- Leaves the app registered in userspace_apps and its files on disk.
The inline comment ("Re-installs of container apps are accepted and stored, but not deployed") contradicts the actual behavior — the caller sees a server error, not a success. Worse, the now-persisted container app appears in list_installed(); fetchUserspaceApps()/toAppManifest map every row (regardless of app_type) to the web SandboxedAppWindow, so a container package would be served/launched as a web iframe. The intended "web-only, reject container" boundary should fail fast and cleanly before any state is mutated, and ideally surface a 501/400 rather than a raw 500.
Reject container packages before store.install() with a proper status code. (Consider also cleaning up the extracted dir, or validating app_type before extract_package.):
try:
manifest = extract_package(data, apps_root=_apps_root(request))
except PackageError as exc:
return JSONResponse({"error": str(exc)}, status_code=400)
if manifest["app_type"] == "container":
# Web-only for now: reject before mutating any state.
return JSONResponse(
{"error": "container packages: web-only for now"},
status_code=501,
)
existing = await store.get(manifest["id"])
...
Was this helpful? React with 👍 / 👎
| @router.post("/api/userspace-apps/install") | ||
| async def install_app(request: Request, package: UploadFile | None = File(default=None)): | ||
| store = request.app.state.userspace_apps | ||
| if package is not None: | ||
| data = await package.read() | ||
| else: | ||
| body = await request.json() | ||
| url = body.get("source_url") | ||
| if not url: | ||
| return JSONResponse({"error": "source_url or package required"}, status_code=400) | ||
| # SSRF guard: only fetch public http(s) hosts, and do not follow | ||
| # redirects (a 3xx could bounce to a blocked internal address). | ||
| if not is_safe_public_url(url): | ||
| return JSONResponse( | ||
| {"error": "source_url is not allowed -- only public http(s) hosts " |
There was a problem hiding this comment.
⚠️ Security: Install endpoint has no upload/zip size limits (zip-bomb DoS)
install_app reads the entire upload into memory with data = await package.read() (or data = resp.content for source_url), then extract_package builds a zipfile.ZipFile(io.BytesIO(data)) and writes every member via dest.write_bytes(zf.read(member)) with no checks on the compressed size, uncompressed size, member count, or total extracted bytes. An authenticated user can submit a small zip-bomb (or a multi-GB upload) and exhaust memory/disk on the host. Even though the endpoint is auth-gated, a single compromised/over-privileged session can take the service down. Add limits: cap the raw upload size, cap per-member and total uncompressed size (using ZipInfo.file_size), and cap member count before extracting.
Bound member count and total uncompressed size before extraction; also enforce a max raw upload size on the route.:
_MAX_TOTAL_UNCOMPRESSED = 100 * 1024 * 1024 # 100 MiB
_MAX_MEMBERS = 5000
# in extract_package, before the write loop:
infos = [i for i in zf.infolist() if not i.filename.endswith("/")]
if len(infos) > _MAX_MEMBERS:
raise PackageError("package has too many files")
total = sum(i.file_size for i in infos)
if total > _MAX_TOTAL_UNCOMPRESSED:
raise PackageError("package expands to too large a size")
Was this helpful? React with 👍 / 👎
| if capability in ("app.files.read", "app.files.write"): | ||
| files_root = (Path(app_dir) / "files").resolve() | ||
| target = (files_root / args.get("path", "")).resolve() | ||
| if not str(target).startswith(str(files_root) + "/") and target != files_root: | ||
| return {"error": "invalid_path"} | ||
| if capability == "app.files.read": | ||
| if not target.is_file(): | ||
| return {"error": "not_found"} | ||
| return {"result": target.read_text()} | ||
| target.parent.mkdir(parents=True, exist_ok=True) | ||
| target.write_text(args.get("content", "")) | ||
| return {"result": True} |
There was a problem hiding this comment.
💡 Edge Case: app.files.write to the jail root raises uncaught IsADirectoryError
In handle_capability the path guard accepts the case target == files_root (the jail root directory itself). For app.files.read that is harmless (is_file() is False -> not_found). But for app.files.write with an empty/'.' path, target resolves to files_root, and the code does target.parent.mkdir(...) then target.write_text(...) on a directory, raising IsADirectoryError. Since the broker route does not wrap handle_capability in a try/except, this surfaces as an unhandled 500 instead of a clean {"error": ...}. Reject writes whose target is the jail root (or any existing directory).
Reject writes that target a directory / the jail root.:
if capability == "app.files.read":
if not target.is_file():
return {"error": "not_found"}
return {"result": target.read_text()}
if target.is_dir() or target == files_root:
return {"error": "invalid_path"}
target.parent.mkdir(parents=True, exist_ok=True)
target.write_text(args.get("content", ""))
return {"result": True}
Was this helpful? React with 👍 / 👎
|
Note Your trial team has used its Gitar budget, so automatic reviews are paused. Upgrade now to unlock full capacity. Comment "Gitar review" to trigger a review manually. Code Review
|
| Compact |
|
Was this helpful? React with 👍 / 👎 | Gitar
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 9
🧹 Nitpick comments (6)
tests/userspace/test_url_guard.py (1)
8-11: ⚡ Quick winConsider adding IPv6 test coverage.
The tests cover IPv4 private/loopback addresses well, but IPv6 equivalents like
::1(loopback) andfc00::/7(unique local) are not tested. Sinceip_address()handles IPv6 and attackers may use IPv6 literals to bypass guards, adding tests would strengthen confidence.def test_blocks_ipv6_loopback_and_private(): assert is_safe_public_url("http://[::1]/x") is False assert is_safe_public_url("http://[fc00::1]/x") is False🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@tests/userspace/test_url_guard.py` around lines 8 - 11, The test_blocks_loopback_and_private function only tests IPv4 addresses, leaving IPv6 loopback and private address ranges untested. Add a new test function (for example, test_blocks_ipv6_loopback_and_private) that tests the is_safe_public_url function with IPv6 loopback addresses like [::1] and IPv6 unique local addresses like [fc00::1] to ensure the function properly rejects these IPv6 equivalents of private ranges using the correct IPv6 literal notation with brackets.tinyagentos/userspace/broker.py (2)
36-52: ⚖️ Poor tradeoffMissing key in
argsraises unhandledKeyError.Several capability handlers access required keys directly (e.g.,
args["key"],args["table"],args["id"]) without guards. If the SDK or a malformed request omits these, the broker raisesKeyError, resulting in a 500 response rather than a structured error.Consider validating required args upfront or using
.get()with explicit error handling:if capability == "app.kv.get": key = args.get("key") if key is None: return {"error": "missing_arg", "arg": "key"} return {"result": await data_store.kv_get(app_id, key)}This is low priority since the SDK controls the args, but improves robustness against malformed requests.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@tinyagentos/userspace/broker.py` around lines 36 - 52, The capability handlers for app.kv.get, app.kv.set, app.kv.delete, app.table.insert, app.table.query, and app.table.delete access required arguments directly from the args dictionary without validation, which causes unhandled KeyError exceptions if keys are missing. For each handler, replace direct dictionary access (e.g., args["key"], args["table"], args["id"]) with args.get() calls and add explicit None checks. When a required argument is missing, return a structured error response with an "error" field and "arg" field indicating which argument is missing, rather than letting the KeyError propagate.
97-99: ⚡ Quick winURL-encoded path traversal may bypass the
..check.The check
".." in path.split("/")catches literal..segments but not URL-encoded variants like%2e%2e. While most backends don't decode these at the path level, some normalize URLs before routing. Consider also rejecting paths containing%2eor usingurllib.parse.unquotebefore validation.+from urllib.parse import unquote ... path = str(args.get("path", "/")) - if "://" in path or path.startswith("//") or ".." in path.split("/"): + decoded = unquote(path) + if "://" in path or path.startswith("//") or ".." in decoded.split("/"): return {"error": "invalid_path"}The current risk is limited since the proxy targets only the app's own backend, but this strengthens defense-in-depth.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@tinyagentos/userspace/broker.py` around lines 97 - 99, The path validation logic in the broker.py file checks for literal ".." segments but does not account for URL-encoded variants like %2e%2e which decode to path traversal sequences. To fix this, decode the URL-encoded path using urllib.parse.unquote before performing the existing validation checks for "://", "//", and ".." in the path. This ensures that both literal and encoded path traversal attempts are properly rejected when validating the path parameter from args.tinyagentos/userspace/data_store.py (2)
54-60: 💤 Low value
table_insertreturn type may need a guard for type safety.
cur.lastrowidis typed asint | Nonein aiosqlite. While a successfulINSERTalways sets it, the return type annotation-> intcould cause type checker warnings. A minor defensive assertion or cast would align the runtime guarantee with the type system.await self._db.commit() - return cur.lastrowid + assert cur.lastrowid is not None + return cur.lastrowid🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@tinyagentos/userspace/data_store.py` around lines 54 - 60, The table_insert method has a type safety issue where cur.lastrowid is typed as int | None in aiosqlite, but the method's return type annotation is -> int. Add either a defensive assertion or type cast on the cur.lastrowid value before returning it to ensure the return type matches the annotation and satisfies type checkers. This aligns the type system with the runtime guarantee that a successful INSERT operation always sets lastrowid.
62-73: ⚖️ Poor tradeoffIn-memory filtering in
table_querymay not scale.The current implementation fetches all rows for an app/table and filters in Python. For apps with many rows, this becomes inefficient. Consider translating simple
whereconditions to SQLWHEREclauses in a future iteration.This is acceptable for phase 1 with small datasets.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@tinyagentos/userspace/data_store.py` around lines 62 - 73, The `table_query` method currently fetches all rows from the database and filters them in Python using the `where` parameter, which is inefficient for large datasets. Modify the method to translate the `where` dictionary into SQL WHERE clause conditions and dynamically build the SELECT query to filter at the database level. When `where` is provided, construct additional AND conditions for each key-value pair in the dictionary, bind them as query parameters to prevent SQL injection, and execute the query with these conditions included. Remove the Python-level filtering logic that checks `where` conditions after fetching rows.tinyagentos/userspace/sdk/taos-app-sdk.js (1)
7-14: 💤 Low valueMessage listener doesn't verify origin.
The
messageevent handler accepts replies from any origin. While the sandbox environment limits exposure and server-side validation provides the real security boundary, verifyinge.originmatches the expected parent would add defense-in-depth.window.addEventListener("message", (e) => { if (e.source !== parent) return; // already good implicit check via parent ref // Optionally also check: if (e.origin !== expectedParentOrigin) return; ... });Low priority since the critical security is server-side.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@tinyagentos/userspace/sdk/taos-app-sdk.js` around lines 7 - 14, The message event listener in the window.addEventListener("message") callback does not verify the origin of incoming messages, which is a security concern. Add origin verification at the beginning of the handler before processing the message: check that e.source matches parent (implicit check via parent reference) and optionally verify that e.origin matches an expected parent origin value. If either check fails, return early to reject the message. This adds a defense-in-depth security layer by ensuring replies are only accepted from trusted origins before resolving pending requests.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@desktop/src/apps/SandboxedAppWindow.tsx`:
- Around line 24-36: The code forwards msg.args directly from the postMessage
without validating that it is a plain object, which can cause the backend broker
to fail with 5xx errors when receiving non-object values. Before using msg.args
in the body JSON.stringify, normalize it by checking if it is actually a plain
object and default to an empty object if it is not, ensuring the broker always
receives valid object arguments at the bridge boundary.
In `@tests/userspace/test_routes.py`:
- Around line 43-44: The path traversal test in the client.get() call uses
literal `../../../` sequences which may be normalized and collapsed before
reaching the route handler, allowing the test to pass with a 404 without
actually validating the server-side bundle path guard. Replace the literal
traversal segments in the URL with URL-encoded equivalents (such as %2e%2e%2f
for ../) so that the encoded path reaches the server and the path guard
validation actually exercises the intended protection mechanism.
In `@tinyagentos/routes/userspace_apps.py`:
- Around line 125-127: The path containment checks at lines 127, 136, and 151
use non-portable string comparison with startswith(str(root) + "/"), which fails
on Windows systems due to different path separators. Replace all three instances
of this pattern with the pathlib-aware method is_relative_to(root), which safely
checks if app_dir is contained within root regardless of the operating system.
This will ensure the security checks work correctly across all platforms by
using proper path comparison semantics instead of string matching.
- Around line 83-93: The store.install() call persists the app before validating
the app_type, and then the NotImplementedError for container types is raised
afterward, leaving a partially installed app in storage. Move the container app
type validation check (comparing manifest["app_type"] to "container") to occur
before the store.install() call. This ensures that if the app is a container
type, the NotImplementedError is raised before any persistence happens,
maintaining install contract consistency.
- Around line 58-73: Add explicit error handling in this endpoint function to
catch two failure scenarios that currently result in 500 errors: (1) wrap the
await request.json() call in a try-except block to catch JSON parsing exceptions
and return a JSONResponse with a 400 status code for malformed JSON, and (2)
wrap the httpx client operations (the c.get(url) call and subsequent
raise_for_status() within the AsyncClient context manager) in a try-except block
to catch HTTP-related exceptions (such as connection errors, timeouts, or HTTP
errors) and return appropriate JSONResponse error responses with suitable status
codes instead of allowing these exceptions to propagate as unhandled 500 errors.
In `@tinyagentos/userspace/package.py`:
- Around line 63-64: The path containment checks using string startswith at
lines 63 and 70 are not robust across different operating systems due to
path-separator differences. Replace both instances of the startswith check with
Path.is_relative_to method calls to properly verify that app_dir is contained
within apps_root. This approach uses pathlib's built-in path handling to ensure
cross-platform compatibility instead of relying on string prefix matching.
- Around line 66-73: The path validation logic in the zip extraction loop allows
members to resolve to the app_dir root directory itself (such as a crafted
member like "."), which causes dest.write_bytes() to fail with IsADirectoryError
since it cannot write bytes to a directory. Modify the condition checking `if
not str(dest).startswith(str(app_dir) + "/") and dest != app_dir:` to properly
reject members that resolve exactly to app_dir by treating them as unsafe paths
and raising a PackageError, ensuring all file extraction targets are strictly
subdirectories of app_dir, not the root directory itself.
- Around line 19-24: After yaml.safe_load returns data on line 20, add a type
check to ensure data is a dictionary before attempting to access the .get()
method in the required field validation loop. If data is not a dictionary (e.g.,
it's a list or scalar), raise a PackageError with a message indicating the
manifest root must be a YAML object. This validation must occur before the for
loop that iterates over _REQUIRED to prevent AttributeError from bubbling up as
a 500 error instead of being properly caught as a PackageError.
In `@tinyagentos/userspace/sdk/taos-app-sdk.js`:
- Line 41: The net.fetch function sends a parameter named url to the broker, but
the broker at line 97 in broker.py expects a parameter named path. This mismatch
causes the url parameter to be ignored and all requests default to the backend
root path. Change the parameter name from url to path in the net.fetch call to
align with what the broker expects, ensuring the requested path is properly
passed through instead of being ignored.
---
Nitpick comments:
In `@tests/userspace/test_url_guard.py`:
- Around line 8-11: The test_blocks_loopback_and_private function only tests
IPv4 addresses, leaving IPv6 loopback and private address ranges untested. Add a
new test function (for example, test_blocks_ipv6_loopback_and_private) that
tests the is_safe_public_url function with IPv6 loopback addresses like [::1]
and IPv6 unique local addresses like [fc00::1] to ensure the function properly
rejects these IPv6 equivalents of private ranges using the correct IPv6 literal
notation with brackets.
In `@tinyagentos/userspace/broker.py`:
- Around line 36-52: The capability handlers for app.kv.get, app.kv.set,
app.kv.delete, app.table.insert, app.table.query, and app.table.delete access
required arguments directly from the args dictionary without validation, which
causes unhandled KeyError exceptions if keys are missing. For each handler,
replace direct dictionary access (e.g., args["key"], args["table"], args["id"])
with args.get() calls and add explicit None checks. When a required argument is
missing, return a structured error response with an "error" field and "arg"
field indicating which argument is missing, rather than letting the KeyError
propagate.
- Around line 97-99: The path validation logic in the broker.py file checks for
literal ".." segments but does not account for URL-encoded variants like %2e%2e
which decode to path traversal sequences. To fix this, decode the URL-encoded
path using urllib.parse.unquote before performing the existing validation checks
for "://", "//", and ".." in the path. This ensures that both literal and
encoded path traversal attempts are properly rejected when validating the path
parameter from args.
In `@tinyagentos/userspace/data_store.py`:
- Around line 54-60: The table_insert method has a type safety issue where
cur.lastrowid is typed as int | None in aiosqlite, but the method's return type
annotation is -> int. Add either a defensive assertion or type cast on the
cur.lastrowid value before returning it to ensure the return type matches the
annotation and satisfies type checkers. This aligns the type system with the
runtime guarantee that a successful INSERT operation always sets lastrowid.
- Around line 62-73: The `table_query` method currently fetches all rows from
the database and filters them in Python using the `where` parameter, which is
inefficient for large datasets. Modify the method to translate the `where`
dictionary into SQL WHERE clause conditions and dynamically build the SELECT
query to filter at the database level. When `where` is provided, construct
additional AND conditions for each key-value pair in the dictionary, bind them
as query parameters to prevent SQL injection, and execute the query with these
conditions included. Remove the Python-level filtering logic that checks `where`
conditions after fetching rows.
In `@tinyagentos/userspace/sdk/taos-app-sdk.js`:
- Around line 7-14: The message event listener in the
window.addEventListener("message") callback does not verify the origin of
incoming messages, which is a security concern. Add origin verification at the
beginning of the handler before processing the message: check that e.source
matches parent (implicit check via parent reference) and optionally verify that
e.origin matches an expected parent origin value. If either check fails, return
early to reject the message. This adds a defense-in-depth security layer by
ensuring replies are only accepted from trusted origins before resolving pending
requests.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro Plus
Run ID: 04ac37ef-7593-4876-8f62-586c0669c808
📒 Files selected for processing (29)
desktop/src/apps/SandboxedAppWindow.tsxdesktop/src/apps/__tests__/SandboxedAppWindow.test.tsxdesktop/src/lib/__tests__/userspace-apps.test.tsdesktop/src/lib/userspace-apps.tsdesktop/src/registry/app-registry.tstests/userspace/__init__.pytests/userspace/conftest.pytests/userspace/test_broker.pytests/userspace/test_broker_route.pytests/userspace/test_data_store.pytests/userspace/test_e2e.pytests/userspace/test_immutability.pytests/userspace/test_install_security.pytests/userspace/test_package.pytests/userspace/test_routes.pytests/userspace/test_sdk_route.pytests/userspace/test_store.pytests/userspace/test_update_consent.pytests/userspace/test_url_guard.pytinyagentos/app.pytinyagentos/routes/__init__.pytinyagentos/routes/userspace_apps.pytinyagentos/userspace/__init__.pytinyagentos/userspace/broker.pytinyagentos/userspace/data_store.pytinyagentos/userspace/package.pytinyagentos/userspace/sdk/taos-app-sdk.jstinyagentos/userspace/store.pytinyagentos/userspace/url_guard.py
![kilo-code-bot[bot]](https://avatars.githubusercontent.com/in/2193792?s=60&v=4){kind=small}
| data = yaml.safe_load(text) or {} | ||
| except yaml.YAMLError as exc: | ||
| raise PackageError(f"invalid manifest YAML: {exc}") from exc | ||
| for key in _REQUIRED: |
There was a problem hiding this comment.
WARNING: Manifest fields are not type-validated before filesystem use
parse_manifest accepts any YAML scalar for id, name, version, or app_type. A numeric id reaches extract_package() and can raise TypeError; non-string path-like values can also bypass the intended manifest contract. Validate required fields are strings and validate id path safety before using them.
| raise PackageError("container app requires container.ports as a non-empty list of ints") | ||
| data.setdefault("entry", "index.html") | ||
| data.setdefault("icon", "") | ||
| data.setdefault("permissions", []) |
There was a problem hiding this comment.
WARNING: permissions manifest value is not validated
permissions is defaulted without checking it is a list of strings. A string permission value is iterable as characters, and arbitrary YAML values can be serialized into the DB. Validate permissions as list[str] before returning the manifest.
| # app_dir itself must stay within apps_root (defends against a crafted id) | ||
| if not str(app_dir).startswith(str(apps_root) + "/"): | ||
| raise PackageError(f"unsafe path in package: id {manifest['id']!r}") | ||
| app_dir.mkdir(parents=True, exist_ok=True) |
There was a problem hiding this comment.
WARNING: Reinstalling a package does not remove stale files
extract_package() creates app_dir and overwrites files in the zip, but leaves files that were removed from the new package. A reinstall can therefore keep old bundle code/assets accessible under the same app id. Clear the validated app directory before extracting, or atomically swap a fresh directory.
| if not str(dest).startswith(str(app_dir) + "/") and dest != app_dir: | ||
| raise PackageError(f"unsafe path in package: {member}") | ||
| dest.parent.mkdir(parents=True, exist_ok=True) | ||
| dest.write_bytes(zf.read(member)) |
There was a problem hiding this comment.
WARNING: Package extraction has no size limit
The route accepts arbitrary multipart or source_url bytes and extract_package() writes every zip member without bounding total or per-file size. A malicious package can exhaust disk space or create very large bundle files. Add explicit package/member size limits before extraction.
| status_code=400, | ||
| ) | ||
| async with httpx.AsyncClient(timeout=120, follow_redirects=False) as c: | ||
| resp = await c.get(url) |
There was a problem hiding this comment.
WARNING: source_url fetch errors are unhandled
resp.raise_for_status() can raise for 4xx/5xx source_url responses and is not caught, so installing from a URL can return a 500 instead of a controlled 400. Catch httpx.HTTPError and return a JSON error response.
| (app_id, name, version, app_type, entry, icon, | ||
| permissions_requested, permissions_granted, enabled, installed_at) | ||
| VALUES (?,?,?,?,?,?,?,'[]',1,?) | ||
| ON CONFLICT(app_id) DO UPDATE SET |
There was a problem hiding this comment.
WARNING: Reinstall preserves stale permission grants
The upsert updates permissions_requested but does not remove grants that are no longer requested. An app can keep previously granted gated capabilities after reinstalling with a narrower manifest. Reconcile permissions_granted to the intersection of existing grants and the new permissions_requested.
| } catch { | ||
| result = { error: "broker_unreachable" }; | ||
| } | ||
| iframe.contentWindow?.postMessage({ taosAppReply: msg.id, ...result }, "*"); |
There was a problem hiding this comment.
WARNING: Broker replies are posted with targetOrigin "*"
The parent sends capability results to the iframe with "*". If the iframe navigates away, the new child origin could receive broker results. Use a fixed target origin for the known bundle origin when possible, or otherwise tighten the reply channel.
| <iframe | ||
| ref={iframeRef} | ||
| title={appId} | ||
| src={`/api/userspace-apps/${encodeURIComponent(appId)}/bundle/index.html?app=${encodeURIComponent(appId)}`} |
There was a problem hiding this comment.
WARNING: Launcher hard-codes index.html despite manifest entry
The manifest stores entry, but SandboxedAppWindow always loads /bundle/index.html. Packages with a different entry file will install successfully but fail to launch. Pass the app entry path through the registry/manifest and use it in the iframe src.
| const id = ++seq; | ||
| return new Promise((resolve) => { | ||
| pending.set(id, { resolve }); | ||
| parent.postMessage({ taosApp: APP_ID, id, capability, args: args || {} }, "*"); |
There was a problem hiding this comment.
WARNING: SDK posts capability requests with targetOrigin "*"
The userspace SDK sends broker requests to parent.postMessage(..., "*"). If the iframe ever navigates or is replaced, another origin could receive capability requests. Prefer a fixed targetOrigin for the parent host origin.
| }, | ||
| notify: (title, body) => call("app.notify", { title, body }), | ||
| // gated -- resolve to {error:"permission_denied"} if not granted | ||
| net: { fetch: (url, opts) => call("app.net", { url, opts }) }, |
There was a problem hiding this comment.
WARNING: net.fetch(url) argument is ignored by the broker
The SDK exposes taos.net.fetch(url, opts), but broker.py's app.net handler ignores url and only accepts path. This makes the SDK API misleading and can cause unexpected failures for callers using the public method. Either pass url to the broker or remove/rename this method.
Code Review SummaryStatus: 19 Issues Found | Recommendation: Address before merge Overview
Fix these issues in Kilo Cloud Resolved from previous review: Issue Details (click to expand)CRITICAL
WARNING
Other Observations (not in diff)Issues found in unchanged code that cannot receive inline comments:
Files Reviewed (11 files including carried-forward unchanged files)
Previous Review Summary (commit fc2671d)Current summary above is authoritative. Previous snapshots are kept for context only. Previous review (commit fc2671d)Status: 21 Issues Found | Recommendation: Address before merge Overview
Fix these issues in Kilo Cloud Issue Details (click to expand)CRITICAL
WARNING
Other Observations (not in diff)No additional issues found outside diff hunks. Files Reviewed (29 files)
Reviewed by nex-n2-pro:free · 761,420 tokens |
…il root (#89 P3a) - package.py: cap declared uncompressed total, per-member size, and member count before extraction (zip-bomb defense, gitar Security finding) - install_app: bound the upload/fetch read to a max size, return 413 if exceeded - broker.py: app.files.write to the jail root or any existing dir now returns invalid_path instead of an uncaught IsADirectoryError (500); path containment uses is_relative_to - tests for the zip-bomb cap and the jail-root write guard
| body = await request.json() | ||
| except Exception: | ||
| return JSONResponse({"error": "invalid JSON body"}, status_code=400) | ||
| url = body.get("source_url") |
There was a problem hiding this comment.
WARNING: Install JSON body shape is not validated
Malformed JSON is handled, but a valid non-object JSON body such as [] or "x" will make body.get(...) raise AttributeError and return a 500. Validate that the JSON body is an object before reading source_url.
Reply with @kilocode-bot fix it to have Kilo Code address this issue.
| f"package uncompressed size too large ({total_uncompressed} bytes)" | ||
| ) | ||
| try: | ||
| manifest = parse_manifest(zf.read("manifest.yaml").decode("utf-8")) |
There was a problem hiding this comment.
WARNING: Manifest UTF-8 decode errors are not converted to PackageError
zf.read("manifest.yaml").decode("utf-8") can raise UnicodeDecodeError for a valid zip with binary manifest content. The route only catches PackageError, so this becomes a 500 instead of a package validation error.
Reply with @kilocode-bot fix it to have Kilo Code address this issue.
There was a problem hiding this comment.
Actionable comments posted: 2
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (4)
tests/userspace/test_routes.py (1)
32-33:⚠️ Potential issue | 🟡 Minor | ⚡ Quick winCSP assertion is too permissive for a security contract test.
Using
orallows regressions where one critical directive disappears without failing the test.Suggested fix
- assert "frame-ancestors" in csp or "default-src" in csp + assert "frame-ancestors" in csp + assert "default-src" in csp🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@tests/userspace/test_routes.py` around lines 32 - 33, The CSP assertion on line 33 uses `or` logic which allows the test to pass if either "frame-ancestors" or "default-src" is present, creating a security regression risk where one critical directive could be removed without failing the test. Change the assertion to require both directives to be present simultaneously by replacing the `or` operator with `and` in the expression that validates the content-security-policy header.tinyagentos/userspace/package.py (1)
76-79:⚠️ Potential issue | 🟠 Major | ⚡ Quick winHandle non-UTF8
manifest.yamlasPackageErrorinstead of 500.A malformed archive with non-UTF8
manifest.yamlraisesUnicodeDecodeErrorhere and bypasses the route’sPackageError→400 path.Suggested fix
- try: - manifest = parse_manifest(zf.read("manifest.yaml").decode("utf-8")) - except KeyError as exc: - raise PackageError("manifest.yaml missing from package") from exc + try: + manifest_bytes = zf.read("manifest.yaml") + except KeyError as exc: + raise PackageError("manifest.yaml missing from package") from exc + try: + manifest = parse_manifest(manifest_bytes.decode("utf-8")) + except UnicodeDecodeError as exc: + raise PackageError("manifest.yaml must be UTF-8 text") from exc🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@tinyagentos/userspace/package.py` around lines 76 - 79, The try-except block in the manifest reading code currently only catches KeyError (when manifest.yaml is missing), but it does not catch UnicodeDecodeError which can occur when manifest.yaml contains non-UTF8 data. This causes the UnicodeDecodeError to propagate uncaught instead of being converted to PackageError. Modify the except clause to catch both KeyError and UnicodeDecodeError, ensuring both cases raise PackageError with appropriate messages so they are properly handled as 400 errors instead of 500 errors.tinyagentos/routes/userspace_apps.py (2)
149-153:⚠️ Potential issue | 🔴 Critical | ⚡ Quick winConstrain
app_idbefore serving bundle files.Line 153 only proves
targetis inside the already-resolvedroot; it does not proverootis a real app directory under the apps root. Withapp_idsuch as.or..via path-as-is/encoded requests, this can expose another app’s files or data-dir files. Check the installed/enabled app first and validaterootagainst the apps root.Proposed fix
`@router.get`("/api/userspace-apps/{app_id}/bundle/{path:path}") async def serve_bundle(request: Request, app_id: str, path: str): - root = (_apps_root(request) / app_id).resolve() + app = await request.app.state.userspace_apps.get(app_id) + if app is None or not app["enabled"]: + return JSONResponse({"error": "not found"}, status_code=404) + + apps_root = _apps_root(request).resolve() + root = (apps_root / app_id).resolve() + if not root.is_relative_to(apps_root) or root == apps_root: + return JSONResponse({"error": "not found"}, status_code=404) + target = (root / path).resolve() if not target.is_relative_to(root) or target == root or not target.is_file(): return JSONResponse({"error": "not found"}, status_code=404)🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@tinyagentos/routes/userspace_apps.py` around lines 149 - 153, The serve_bundle function validates that the target file is within the root directory, but does not validate that the root directory itself is a legitimate app directory under the apps root. This allows an attacker to use path traversal in app_id (such as "." or "..") to access other apps' files. Before resolving and serving the bundle, validate that app_id corresponds to an installed/enabled app, and verify that the resolved root path (created from _apps_root(request) / app_id) is actually within the apps root directory by checking that root is relative to _apps_root(request). Add this validation logic before attempting to serve the target file.
71-79:⚠️ Potential issue | 🔴 CriticalDNS rebinding can bypass the SSRF guard — pin the resolved IP before fetching.
is_safe_public_url(url)validates hostnames by resolving them once, buthttpx.AsyncClient.get(url)resolves the hostname again at request time. An attacker controlling DNS can return a public IP during validation and a private address during the actual fetch, bypassing the guard. Mitigate by either:
- Caching the validated IP and passing it directly to httpx (via custom socket or IP literal in URL)
- Enforcing egress filtering that blocks private IPs at the network layer
- Using a custom httpx Transport with a pinned resolver that reuses the validated IP
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@tinyagentos/routes/userspace_apps.py` around lines 71 - 79, The DNS rebinding vulnerability occurs because is_safe_public_url validates the hostname by resolving it once, but httpx.AsyncClient.get resolves it again during the actual fetch, allowing an attacker to swap the IP between validation and request. To fix this, modify the code to capture and reuse the validated IP address instead of re-resolving the hostname. Either resolve the URL's hostname before calling is_safe_public_url, pass the validated IP directly to the httpx client (by using it in the URL or via a custom transport that binds to that specific IP), or implement a custom httpx Transport that pins the resolver to the previously validated IP. Ensure the resolved IP is used for the httpx request rather than allowing httpx to perform its own DNS resolution.
♻️ Duplicate comments (4)
tinyagentos/routes/userspace_apps.py (4)
195-198:⚠️ Potential issue | 🟠 Major | ⚡ Quick winValidate broker request shape before dispatch.
Malformed JSON, non-object JSON, non-string
capability, or non-objectargscan still reach.get()/handle_capability()and produce 500s instead of a controlled broker error.Proposed fix
- body = await request.json() + try: + body = await request.json() + except ValueError: + return JSONResponse({"error": "invalid JSON body"}, status_code=400) + if not isinstance(body, dict): + return JSONResponse({"error": "invalid_args"}, status_code=400) + capability = body.get("capability", "") + args = body.get("args") or {} + if not isinstance(capability, str) or not isinstance(args, dict): + return JSONResponse({"error": "invalid_args"}, status_code=400) out = await handle_capability( - app_id, body.get("capability", ""), body.get("args") or {}, + app_id, capability, args, granted=app["permissions_granted"],🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@tinyagentos/routes/userspace_apps.py` around lines 195 - 198, After the await request.json() call that assigns to body, add validation to ensure body is a dictionary, capability is a string, and args is a dictionary (if provided). If validation fails, return a controlled error response instead of allowing malformed values to reach the handle_capability() function. This prevents 500 errors from invalid types and ensures proper error handling for the broker request.
91-100:⚠️ Potential issue | 🟠 Major | 🏗️ Heavy liftReject containers before extraction mutates the app directory.
This now avoids the DB insert, but
extract_package()has already written intoapps_root/{manifest["id"]}before Line 96. A rejected container install can still leave orphan files or overwrite an existing app’s bundle before returning 501. Peek/parse the manifest before extraction, or extract into a temporary staging directory and only move it into place after the web-only check passes.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@tinyagentos/routes/userspace_apps.py` around lines 91 - 100, The extract_package() function is being called before validating that the app_type is not a container, which means files are already written to the apps_root directory before the 501 rejection is returned, potentially leaving orphaned files or overwriting existing apps. Restructure the code to either parse the manifest metadata before calling extract_package() to validate the app_type constraint upfront, or modify extract_package() to extract into a temporary staging directory first, then only move the extracted files into the permanent apps_root location after confirming the container check passes.
119-123:⚠️ Potential issue | 🟠 Major | ⚡ Quick winValidate grants against the app’s requested permissions.
This endpoint still accepts arbitrary
grantedvalues and writes them into the authorization set used by the broker. Fetch the app row, require a list of strings, and reject any permission not present inpermissions_requested.Proposed fix
`@router.post`("/api/userspace-apps/{app_id}/permissions") async def set_permissions(request: Request, app_id: str): - body = await request.json() - await request.app.state.userspace_apps.set_permissions_granted(app_id, body.get("granted", [])) + try: + body = await request.json() + except ValueError: + return JSONResponse({"error": "invalid JSON body"}, status_code=400) + if not isinstance(body, dict): + return JSONResponse({"error": "invalid JSON body"}, status_code=400) + + app = await request.app.state.userspace_apps.get(app_id) + if app is None: + return JSONResponse({"error": "app not found"}, status_code=404) + + granted = body.get("granted", []) + if not isinstance(granted, list) or not all(isinstance(p, str) for p in granted): + return JSONResponse({"error": "granted must be a list of permissions"}, status_code=400) + + requested = set(app["permissions_requested"]) + if any(p not in requested for p in granted): + return JSONResponse({"error": "cannot grant unrequested permissions"}, status_code=400) + + await request.app.state.userspace_apps.set_permissions_granted(app_id, granted) return {"status": "ok"}🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@tinyagentos/routes/userspace_apps.py` around lines 119 - 123, The set_permissions endpoint accepts arbitrary permission values without validation. Fetch the app record using app_id to retrieve its permissions_requested field, then validate that the granted list from the request body contains only strings and that each granted permission exists in the app's permissions_requested. Reject the request with an appropriate error response if the granted list is not a list of strings or if any permission is not in the app's requested permissions, only proceeding to call set_permissions_granted if all validations pass.
77-90:⚠️ Potential issue | 🟠 Major | ⚡ Quick winStream
source_urldownloads before buffering.Line 79/81 still reads the full upstream response into memory before Line 89 enforces
_MAX_PACKAGE_BYTES; a public attacker-controlled URL can return a huge body and bypass the intended memory cap until after allocation. This is the residual fetch-side part of the previous size-limit finding.Proposed fix
try: async with httpx.AsyncClient(timeout=120, follow_redirects=False) as c: - resp = await c.get(url) - resp.raise_for_status() - data = resp.content + async with c.stream("GET", url) as resp: + resp.raise_for_status() + chunks: list[bytes] = [] + total = 0 + async for chunk in resp.aiter_bytes(): + total += len(chunk) + if total > _MAX_PACKAGE_BYTES: + return JSONResponse({"error": "package too large"}, status_code=413) + chunks.append(chunk) + data = b"".join(chunks)🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@tinyagentos/routes/userspace_apps.py` around lines 77 - 90, The current implementation reads the entire upstream response into memory via resp.content on line 81 before checking the _MAX_PACKAGE_BYTES limit on line 89, which allows an attacker to exhaust memory with a huge response body. Instead of buffering the full response content at once, stream the response data incrementally and apply the size limit check as data arrives. Modify the httpx request to read the response in chunks (using iter_bytes or iter_raw), accumulate the data while tracking the total size, and raise an exception or return the 413 error response as soon as the size exceeds _MAX_PACKAGE_BYTES, preventing the full body from being loaded into memory.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@tests/userspace/test_routes.py`:
- Around line 91-103: The test_container_install_rejected_with_no_stored_state
function only validates that no database row is stored but does not verify that
the rejected container package files are properly cleaned up from the filesystem
and cannot be served. After asserting that no app row exists in the database,
add additional assertions to verify that the extracted/installed container files
are not accessible or servable from the /bundle/... endpoint, ensuring complete
rollback of both database and filesystem state when container installation is
rejected.
In `@tinyagentos/routes/userspace_apps.py`:
- Around line 62-66: The exception handler in the try-except block around await
request.json() is too broad with its generic Exception catch, and it does not
validate that the JSON body is actually a dictionary before calling .get() on
it. Replace the broad except Exception with more specific exceptions like
JSONDecodeError or ValueError that are raised during JSON parsing failures.
Additionally, after obtaining the body from await request.json(), add validation
to check that body is a dictionary before attempting to access
body.get("source_url"). If body is not a dict, return a JSONResponse with a 400
status code indicating that the JSON body must be an object. This prevents
AttributeError exceptions when the parsed JSON is a list or string and ensures
only properly formatted JSON objects are processed.
---
Outside diff comments:
In `@tests/userspace/test_routes.py`:
- Around line 32-33: The CSP assertion on line 33 uses `or` logic which allows
the test to pass if either "frame-ancestors" or "default-src" is present,
creating a security regression risk where one critical directive could be
removed without failing the test. Change the assertion to require both
directives to be present simultaneously by replacing the `or` operator with
`and` in the expression that validates the content-security-policy header.
In `@tinyagentos/routes/userspace_apps.py`:
- Around line 149-153: The serve_bundle function validates that the target file
is within the root directory, but does not validate that the root directory
itself is a legitimate app directory under the apps root. This allows an
attacker to use path traversal in app_id (such as "." or "..") to access other
apps' files. Before resolving and serving the bundle, validate that app_id
corresponds to an installed/enabled app, and verify that the resolved root path
(created from _apps_root(request) / app_id) is actually within the apps root
directory by checking that root is relative to _apps_root(request). Add this
validation logic before attempting to serve the target file.
- Around line 71-79: The DNS rebinding vulnerability occurs because
is_safe_public_url validates the hostname by resolving it once, but
httpx.AsyncClient.get resolves it again during the actual fetch, allowing an
attacker to swap the IP between validation and request. To fix this, modify the
code to capture and reuse the validated IP address instead of re-resolving the
hostname. Either resolve the URL's hostname before calling is_safe_public_url,
pass the validated IP directly to the httpx client (by using it in the URL or
via a custom transport that binds to that specific IP), or implement a custom
httpx Transport that pins the resolver to the previously validated IP. Ensure
the resolved IP is used for the httpx request rather than allowing httpx to
perform its own DNS resolution.
In `@tinyagentos/userspace/package.py`:
- Around line 76-79: The try-except block in the manifest reading code currently
only catches KeyError (when manifest.yaml is missing), but it does not catch
UnicodeDecodeError which can occur when manifest.yaml contains non-UTF8 data.
This causes the UnicodeDecodeError to propagate uncaught instead of being
converted to PackageError. Modify the except clause to catch both KeyError and
UnicodeDecodeError, ensuring both cases raise PackageError with appropriate
messages so they are properly handled as 400 errors instead of 500 errors.
---
Duplicate comments:
In `@tinyagentos/routes/userspace_apps.py`:
- Around line 195-198: After the await request.json() call that assigns to body,
add validation to ensure body is a dictionary, capability is a string, and args
is a dictionary (if provided). If validation fails, return a controlled error
response instead of allowing malformed values to reach the handle_capability()
function. This prevents 500 errors from invalid types and ensures proper error
handling for the broker request.
- Around line 91-100: The extract_package() function is being called before
validating that the app_type is not a container, which means files are already
written to the apps_root directory before the 501 rejection is returned,
potentially leaving orphaned files or overwriting existing apps. Restructure the
code to either parse the manifest metadata before calling extract_package() to
validate the app_type constraint upfront, or modify extract_package() to extract
into a temporary staging directory first, then only move the extracted files
into the permanent apps_root location after confirming the container check
passes.
- Around line 119-123: The set_permissions endpoint accepts arbitrary permission
values without validation. Fetch the app record using app_id to retrieve its
permissions_requested field, then validate that the granted list from the
request body contains only strings and that each granted permission exists in
the app's permissions_requested. Reject the request with an appropriate error
response if the granted list is not a list of strings or if any permission is
not in the app's requested permissions, only proceeding to call
set_permissions_granted if all validations pass.
- Around line 77-90: The current implementation reads the entire upstream
response into memory via resp.content on line 81 before checking the
_MAX_PACKAGE_BYTES limit on line 89, which allows an attacker to exhaust memory
with a huge response body. Instead of buffering the full response content at
once, stream the response data incrementally and apply the size limit check as
data arrives. Modify the httpx request to read the response in chunks (using
iter_bytes or iter_raw), accumulate the data while tracking the total size, and
raise an exception or return the 413 error response as soon as the size exceeds
_MAX_PACKAGE_BYTES, preventing the full body from being loaded into memory.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro Plus
Run ID: b58b358d-7e69-482c-a82a-2a585d028102
📒 Files selected for processing (10)
desktop/node_modulesdesktop/src/apps/SandboxedAppWindow.tsxdesktop/src/apps/__tests__/SandboxedAppWindow.test.tsxtests/userspace/test_broker.pytests/userspace/test_package.pytests/userspace/test_routes.pytinyagentos/routes/userspace_apps.pytinyagentos/userspace/broker.pytinyagentos/userspace/package.pytinyagentos/userspace/sdk/taos-app-sdk.js
✅ Files skipped from review due to trivial changes (1)
- desktop/node_modules
🚧 Files skipped from review as they are similar to previous changes (5)
- desktop/src/apps/tests/SandboxedAppWindow.test.tsx
- desktop/src/apps/SandboxedAppWindow.tsx
- tinyagentos/userspace/sdk/taos-app-sdk.js
- tests/userspace/test_broker.py
- tinyagentos/userspace/broker.py
| async def test_container_install_rejected_with_no_stored_state(client): | ||
| # Finding 3: installing a container package must return 501 before | ||
| # persisting anything -- no app row and no extracted directory must remain. | ||
| r = await client.post( | ||
| "/api/userspace-apps/install", | ||
| files={"package": ("ctapp.taosapp", _container_zip(), "application/zip")}, | ||
| ) | ||
| assert r.status_code == 501 | ||
| assert "container" in r.json()["error"].lower() | ||
|
|
||
| # No app row stored. | ||
| rows = (await client.get("/api/userspace-apps")).json() | ||
| assert all(a["app_id"] != "ctapp" for a in rows) |
There was a problem hiding this comment.
The container-rejection test does not validate filesystem rollback/no-serve behavior.
This test currently checks DB state only, so it can miss a case where rejected container package files are still servable from /bundle/....
Suggested fix
async def test_container_install_rejected_with_no_stored_state(client):
@@
rows = (await client.get("/api/userspace-apps")).json()
assert all(a["app_id"] != "ctapp" for a in rows)
+
+ # Rejected install must not leave bundle files accessible.
+ r = await client.get("/api/userspace-apps/ctapp/bundle/index.html")
+ assert r.status_code == 404🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@tests/userspace/test_routes.py` around lines 91 - 103, The
test_container_install_rejected_with_no_stored_state function only validates
that no database row is stored but does not verify that the rejected container
package files are properly cleaned up from the filesystem and cannot be served.
After asserting that no app row exists in the database, add additional
assertions to verify that the extracted/installed container files are not
accessible or servable from the /bundle/... endpoint, ensuring complete rollback
of both database and filesystem state when container installation is rejected.
| try: | ||
| body = await request.json() | ||
| except Exception: | ||
| return JSONResponse({"error": "invalid JSON body"}, status_code=400) | ||
| url = body.get("source_url") |
There was a problem hiding this comment.
Reject non-object JSON bodies and narrow the exception.
await request.json() can successfully return a list/string, so Line 66 can still 500 on .get; except Exception also masks unrelated failures. Catch JSON decode/value errors and verify body is a dict before reading source_url.
Proposed fix
- try:
- body = await request.json()
- except Exception:
+ try:
+ body = await request.json()
+ except ValueError:
return JSONResponse({"error": "invalid JSON body"}, status_code=400)
+ if not isinstance(body, dict):
+ return JSONResponse({"error": "invalid JSON body"}, status_code=400)
url = body.get("source_url")🧰 Tools
🪛 Ruff (0.15.17)
[warning] 64-64: Do not catch blind exception: Exception
(BLE001)
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@tinyagentos/routes/userspace_apps.py` around lines 62 - 66, The exception
handler in the try-except block around await request.json() is too broad with
its generic Exception catch, and it does not validate that the JSON body is
actually a dictionary before calling .get() on it. Replace the broad except
Exception with more specific exceptions like JSONDecodeError or ValueError that
are raised during JSON parsing failures. Additionally, after obtaining the body
from await request.json(), add validation to check that body is a dictionary
before attempting to access body.get("source_url"). If body is not a dict,
return a JSONResponse with a 400 status code indicating that the JSON body must
be an object. This prevents AttributeError exceptions when the parsed JSON is a
list or string and ensures only properly formatted JSON objects are processed.
Source: Linters/SAST tools
1 participant
Phase P3a of the app system (#89): re-integrate the web userspace app-runtime foundation onto current dev. This is the foundation that real .taosapp app packages run on.
What lands
The App Runtime work (originally PR #476, whose branch was ~598 commits behind dev) re-integrated onto current dev, web-only:
Scope boundaries (deliberate)
Tests
40 backend (tests/userspace/) + 9 frontend (SandboxedAppWindow + userspace-apps) pass; app create_app smoke OK; tsc clean; build green.
Part of #89.
Summary by CodeRabbit
New Features
userspaceapp category.Security