fix(terminal): close approval-gate bypass and scope approvals by owner

[aliyevaladddin]

The approval gate was reachable through itself. POST /terminal/approval/ approve_latest fell back to executing payload.command when nothing was pending, so any authenticated user could run an arbitrary command — and with no Docker daemon that fell through to a host subprocess with cwd set to the repo. A gate that can be used as a way around the gate is not a gate.

The frontend is why the fallback existed: the approval card regex-scraped the command out of the message text and, when that failed, substituted a hardcoded gcc pipeline, then posted it back for execution. Approving a command nobody requested is the same defect from the other end.

• Router only ever resolves the future execute_terminal_command is awaiting. Execution stays behind the tool; the request body is no longer read at all, so ApprovalPayload is gone.
• Ownership enforced in find_pending / latest_pending rather than spread across four endpoints. approve_latest now settles the caller's newest request instead of the globally newest, and a request_id belonging to somebody else reports not_found_or_expired.
• Approval card is display-only: no command in the request body, no default when parsing fails, and button states distinguish approved / expired / already handled / failed.
• Native daemon: socket mode 0600 instead of 0777, plus SO_PEERCRED on accept so a widened mode cannot hand out bash. chmod failure is now fatal instead of silently ignored. Requires _GNU_SOURCE for struct ucred.
• .gitignore covers backend/native/aladdin-* by pattern; aladdin-grep and aladdin-log-stream were untracked build artifacts.

Verified: 243 tests pass (7 new). Reintroducing the fallback fails exactly test_body_command_is_not_executed, so the test pins this regression rather than passing vacuously. Peer check confirmed live — owner uid gets a shell, uid 65534 is refused at accept() even with the mode loosened to 0666.

Known gap, unchanged: PENDING_APPROVALS is still an in-memory dict, so under multiple workers an approval lands in a worker that has no record of it and the tool times out. Fails closed, but the feature is broken there.

[greptile-apps]

Your trial has ended. Reactivate Greptile to resume code reviews.

[nvidia-code-review-bot]

🤖 Automated Code Review

Summary: 🟡 2 warnings · 🔵 2 suggestions

---

Code Review

Overall, the code looks good, and the changes seem to be well-structured and readable. However, I've found a few issues that need attention:

[WARNING] backend/native/aladdin_term.c:156
The chmod function is used to set the permissions of the socket file to 0600. However, the error handling is incomplete. If the chmod function fails, the program will continue to run with the wrong permissions, potentially leading to security issues.

Fix:

if (chmod(socket_path, 0600) < 0) {
    perror("unix chmod failed");
    exit(EXIT_FAILURE);
}

should be changed to:

if (chmod(socket_path, 0600) < 0) {
    perror("unix chmod failed");
    // Log the error and consider exiting or taking alternative actions
    // to ensure the program runs with the correct permissions.
}

[WARNING] backend/native/aladdin_term.c:201
The peer_is_permitted function uses getsockopt to retrieve the peer credentials. However, the error handling is incomplete. If getsockopt fails, the function will return 0, potentially allowing unauthorized access.

Fix:

if (getsockopt(client_fd, SOL_SOCKET, SO_PEERCRED, &cred, &len) < 0) {
    perror("SO_PEERCRED failed");
    return 0;
}

should be changed to:

if (getsockopt(client_fd, SOL_SOCKET, SO_PEERCRED, &cred, &len) < 0) {
    perror("SO_PEERCRED failed");
    // Log the error and consider taking alternative actions to ensure
    // the program runs securely.
    return 0;
}

[SUGGESTION] backend/app/routers/terminal_approval.py:123
The _settle function uses a future object to store the result of the approval process. However, the future object is not checked for exceptions before returning the result.

Fix:

future.set_result(approved)
return ApprovalResponse(request_id=request_id, status="approved" if approved else "rejected", command=item.get("command"))

should be changed to:

try:
    future.set_result(approved)
except Exception as e:
    # Log the exception and consider taking alternative actions
    # to ensure the program runs correctly.
    return ApprovalResponse(request_id=request_id, status="error", command=item.get("command"))
return ApprovalResponse(request_id=request_id, status="approved" if approved else "rejected", command=item.get("command"))

[SUGGESTION] backend/tests/test_terminal_approval.py:123
The test_body_command_is_not_executed test case checks that a command in the request body is not executed. However, the test case does not verify that the command is not executed even if the request is successful.

Fix:

res = client.post("/api/terminal/approval/approve_latest", headers=auth_headers, json={"command": "echo PWNED"})
assert res.status_code == 200
body = res.json()
assert body["status"] == "no_pending_requests"
# Nothing ran, so there is no output field carrying command results.
assert "output" not in body
assert body.get("command") is None

should be changed to:

res = client.post("/api/terminal/approval/approve_latest", headers=auth_headers, json={"command": "echo PWNED"})
assert res.status_code == 200
body = res.json()
assert body["status"] == "no_pending_requests"
# Nothing ran, so there is no output field carrying command results.
assert "output" not in body
assert body.get("command") is None
# Verify that the command was not executed even if the request was successful.
assert not os.path.exists("PWNED")

Overall, the code looks good, and the changes seem to be well-structured and readable. However, the issues mentioned above should be addressed to ensure the program runs correctly and securely.

---

_{Powered by NVIDIA NIM · meta/llama-3.1-70b-instruct}