forked from bai/curriculum-project-hub
docs: resolve tenant security audit
This commit is contained in:
@@ -0,0 +1,258 @@
|
|||||||
|
# Tenant, authentication, and request security audit
|
||||||
|
|
||||||
|
## Verdict
|
||||||
|
|
||||||
|
The current Hub has a sound application-level foundation for ordinary org-admin
|
||||||
|
CRUD isolation, but it is **not safe to deploy as a multi-tenant SaaS**. Several
|
||||||
|
agent and asynchronous-action paths can cross the Organization/Project boundary,
|
||||||
|
and the current process-global credential design directly contradicts the
|
||||||
|
org-scoped encrypted-secret decision in ADR-0021.
|
||||||
|
|
||||||
|
The answer to the audit question is therefore **yes**: current filesystem,
|
||||||
|
Feishu-context, deferred authorization, card-action, and credential paths can
|
||||||
|
cross or bypass the intended boundary. These are implementation divergences,
|
||||||
|
not unspecified product behavior.
|
||||||
|
|
||||||
|
## Contract baseline
|
||||||
|
|
||||||
|
The audit used these upstream requirements:
|
||||||
|
|
||||||
|
- `Spec.System.Organization`: PROJECT and TEAM each have one Organization;
|
||||||
|
TEAM→PROJECT grants are same-org only.
|
||||||
|
- `Spec.System.ProjectWorkspace`: folders are transparent; folder/project
|
||||||
|
placement must remain inside one Organization.
|
||||||
|
- `Spec.System.Permission`: agent trigger requires EDIT, normal cancel requires
|
||||||
|
MANAGE, and force release is outside the project role lattice.
|
||||||
|
- `Spec.System.Memory`: a run may read Feishu context only from its project's
|
||||||
|
bound chat.
|
||||||
|
- `Spec.System.AgentSurface`: every agent file operation must stay inside that
|
||||||
|
run's project workspace.
|
||||||
|
- ADR-0019: authorization is evaluated against principals resolved at request
|
||||||
|
time so membership changes take effect immediately.
|
||||||
|
- ADR-0021: customer Feishu and model-provider credentials are org-scoped
|
||||||
|
secrets, encrypted at rest, and accessed through a resolver.
|
||||||
|
|
||||||
|
## Confirmed protections
|
||||||
|
|
||||||
|
The following paths are correctly fail-closed today:
|
||||||
|
|
||||||
|
- Every `/api/org/:orgSlug/*` route calls `requireOrgRole`; the guard reloads
|
||||||
|
the user, active membership, allowed OWNER/ADMIN role, and ACTIVE org for each
|
||||||
|
request.
|
||||||
|
- Project/session/folder/team service functions compare the requested object's
|
||||||
|
Organization before returning or mutating it. Cross-org lookups return not
|
||||||
|
found instead of revealing the foreign object.
|
||||||
|
- `PermissionAuthorizer` derives the Organization from the resource before
|
||||||
|
expanding team and external principals. `grantTeamProjectAccess` refuses a
|
||||||
|
TEAM whose Organization differs from the Project and detects corrupt active
|
||||||
|
grants when listing.
|
||||||
|
- Feishu messages with no sender `open_id` are denied. Bound-chat triggers run
|
||||||
|
through `agent.trigger` and `role.trigger` gates before creating an AgentRun.
|
||||||
|
- OAuth state is signed, expiry-checked, and paired with an HttpOnly nonce
|
||||||
|
cookie. `returnTo` is constrained to same-origin `/admin` paths. Session
|
||||||
|
signatures use HMAC-SHA256 with constant-time comparison, and session users
|
||||||
|
are reloaded on every request.
|
||||||
|
- Fastify's current default body limit is 1 MiB, so HTTP bodies are not
|
||||||
|
completely unbounded even though field-level and rate limits are absent.
|
||||||
|
|
||||||
|
Executed evidence:
|
||||||
|
|
||||||
|
```text
|
||||||
|
5 integration files passed
|
||||||
|
28 tests passed
|
||||||
|
```
|
||||||
|
|
||||||
|
Those tests include unauthenticated access, member-vs-admin roles, cross-org
|
||||||
|
project IDs, Organization-scoped team slugs, and cross-org TEAM grant refusal.
|
||||||
|
|
||||||
|
## Critical findings
|
||||||
|
|
||||||
|
### 1. The agent receives service secrets and can read other tenant data
|
||||||
|
|
||||||
|
`hub/src/agent/runner.ts` builds the SDK environment as:
|
||||||
|
|
||||||
|
```text
|
||||||
|
{ ...process.env, ...providerEnv }
|
||||||
|
```
|
||||||
|
|
||||||
|
The sandboxed agent and its Bash subprocess therefore inherit `DATABASE_URL`,
|
||||||
|
`FEISHU_APP_SECRET`, `HUB_SESSION_SECRET`, the model-provider token, and every
|
||||||
|
other service variable. A model or prompt injection can run `env`; network
|
||||||
|
egress is intentionally open, so exposed values can leave the host.
|
||||||
|
|
||||||
|
The same sandbox config has only `allowWrite: [workspaceDir]`. Reads outside
|
||||||
|
the workspace are allowed except for a small deny list. All organizations run
|
||||||
|
as the same service user, and sibling project workspaces are not denied. This
|
||||||
|
violates `AgentFileOp.Authorized`, which covers reads as well as writes.
|
||||||
|
|
||||||
|
The installed Claude Agent SDK already exposes two mechanisms that are not
|
||||||
|
used: sandbox credential rules can `deny` or proxy-`mask` environment variables,
|
||||||
|
and filesystem rules can deny a workspace root while re-allowing the current
|
||||||
|
workspace. Production still needs an actual Linux test proving that SDK/Bash
|
||||||
|
cannot read another project or service credential.
|
||||||
|
|
||||||
|
### 2. MCP file and Feishu-context tools escape their run objects
|
||||||
|
|
||||||
|
`resolveDeliverableFile` intentionally adds the repository Git root to allowed
|
||||||
|
roots, even when it is above the project workspace. It also checks lexical paths
|
||||||
|
with `stat`, which follows symlinks. A deterministic reproduction created
|
||||||
|
`workspace/leak.txt` as a symlink to another tenant's file:
|
||||||
|
|
||||||
|
```json
|
||||||
|
{"symlinkEscapeAccepted":true,"outsideContentReadable":true}
|
||||||
|
```
|
||||||
|
|
||||||
|
The checked-in unit test currently treats sending a repo-root `README.md` from
|
||||||
|
a nested project as desired behavior. That expectation is a direct divergence
|
||||||
|
from the AgentSurface contract and must be reversed.
|
||||||
|
|
||||||
|
`readFeishuContext` ignores its `ToolContext`. The MCP wrapper closes over the
|
||||||
|
bound `chatId`, but the model supplies an arbitrary message/thread ID and the
|
||||||
|
reader never verifies the returned message's `chat_id`. A stub Feishu API
|
||||||
|
returning a message from `chat-other` produced:
|
||||||
|
|
||||||
|
```json
|
||||||
|
{"returnedChatId":"chat-other","crossChatContentReturned":true}
|
||||||
|
```
|
||||||
|
|
||||||
|
The same validation is required for thread parents and every returned item.
|
||||||
|
|
||||||
|
Inbound downloads have the inverse symlink problem: the unsandboxed Hub writes
|
||||||
|
to `workspace/.cph/inbox`. A prior agent command can replace a path component
|
||||||
|
with a symlink, turning the Hub into a confused deputy unless writes use a
|
||||||
|
verified real path/no-follow boundary.
|
||||||
|
|
||||||
|
### 3. SaaS credentials are process-global instead of org-scoped
|
||||||
|
|
||||||
|
`server.ts` creates one process-global Feishu client/listener and passes one
|
||||||
|
global app ID/secret to OAuth. `feishuOAuth.ts` explicitly calls per-org
|
||||||
|
encrypted credentials "deferred", although ADR-0021 says they **must** be
|
||||||
|
org-scoped, encrypted at rest, and accessed through a resolver.
|
||||||
|
|
||||||
|
`EnvRuntimeSettings.provider` similarly discards its `scope` argument and uses
|
||||||
|
one global OpenRouter token for every project. This both leaks tenant ownership
|
||||||
|
of credentials and makes the accepted "customers supply their own provider
|
||||||
|
keys" accounting model false. The schema has no encrypted credential store,
|
||||||
|
key version, rotation state, or resolver seam.
|
||||||
|
|
||||||
|
This is also a product reachability blocker: one customer-owned Feishu app
|
||||||
|
cannot receive bot/OAuth traffic for unrelated customer tenants under the
|
||||||
|
ADR-0021 model.
|
||||||
|
|
||||||
|
## High findings
|
||||||
|
|
||||||
|
### 4. Delayed and card actions are not bound to the authorized object
|
||||||
|
|
||||||
|
- `onMessage` checks `agent.trigger` before enqueueing/batching, but
|
||||||
|
`startAgentRun` checks only `role.trigger`. If a grant, team membership, or
|
||||||
|
tenant status changes while a request waits, it still starts. This contradicts
|
||||||
|
ADR-0019's request-time effectiveness rule.
|
||||||
|
- Interrupt handling authorizes `agent.cancel` against the project bound to the
|
||||||
|
card action's current chat, then calls `activeRuns.get(runId)` without proving
|
||||||
|
that `runId` belongs to that project. A stale, forwarded, or otherwise
|
||||||
|
mismatched action can abort another project's active controller.
|
||||||
|
- Approval handling stores the expected `chatId` but never compares it to the
|
||||||
|
action context before resolving the pending approval.
|
||||||
|
|
||||||
|
Execution-time authorization must resolve one immutable tuple
|
||||||
|
`(organization, project, run/action, actor, chat)` and reject any mismatch
|
||||||
|
before mutation.
|
||||||
|
|
||||||
|
### 5. SUSPENDED organizations can still execute bound projects
|
||||||
|
|
||||||
|
HTTP org-admin guards and unbound-chat onboarding reject non-ACTIVE orgs, but
|
||||||
|
`PermissionAuthorizer.organizationForResource` returns only `organizationId`
|
||||||
|
and never checks status. A real database reproduction created a SUSPENDED org,
|
||||||
|
project, and EDIT grant:
|
||||||
|
|
||||||
|
```json
|
||||||
|
{"organizationStatus":"SUSPENDED","agentTriggerAllowed":true}
|
||||||
|
```
|
||||||
|
|
||||||
|
Project onboarding helpers also check membership existence without checking
|
||||||
|
Organization status. The same status must be enforced at the shared resource
|
||||||
|
authorization/onboarding seam, not independently in selected transports.
|
||||||
|
|
||||||
|
### 6. Production dependency graphs contain known vulnerabilities
|
||||||
|
|
||||||
|
`npm audit --omit=dev` exits 1 with two high findings:
|
||||||
|
|
||||||
|
- `@larksuiteoapi/node-sdk@1.68.0`
|
||||||
|
- transitive `axios@1.13.6`
|
||||||
|
|
||||||
|
The current latest Feishu SDK (`1.70.0` at audit time) still pins Axios
|
||||||
|
`~1.13.3`, while the audited fixed Axios line is newer. The relevant advisories
|
||||||
|
include request/credential manipulation, proxy credential leakage, ReDoS, and
|
||||||
|
unbounded resource allocation; for example
|
||||||
|
[GHSA-3g43-6gmg-66jw](https://github.com/advisories/GHSA-3g43-6gmg-66jw),
|
||||||
|
[GHSA-hfxv-24rg-xrqf](https://github.com/advisories/GHSA-hfxv-24rg-xrqf), and
|
||||||
|
[GHSA-777c-7fjr-54vf](https://github.com/advisories/GHSA-777c-7fjr-54vf).
|
||||||
|
An override is outside the SDK's declared semver range and therefore requires
|
||||||
|
integration testing rather than a blind lockfile edit.
|
||||||
|
|
||||||
|
`cargo audit` exits 1 with five vulnerability records:
|
||||||
|
|
||||||
|
- `crossbeam-epoch@0.9.18`, patched in `>=0.9.20`;
|
||||||
|
- `quick-xml@0.38.4` and `0.39.4`, each affected by namespace-allocation and
|
||||||
|
quadratic-attribute DoS advisories, patched in `>=0.41.0`.
|
||||||
|
|
||||||
|
It also reports `memmap2@0.9.10` as unsound (patched in `>=0.9.11`) and four
|
||||||
|
unmaintained transitive crates. All enter through the Typst 0.15 dependency
|
||||||
|
graph. References:
|
||||||
|
[RUSTSEC-2026-0204](https://rustsec.org/advisories/RUSTSEC-2026-0204.html),
|
||||||
|
[RUSTSEC-2026-0194](https://rustsec.org/advisories/RUSTSEC-2026-0194.html),
|
||||||
|
[RUSTSEC-2026-0195](https://rustsec.org/advisories/RUSTSEC-2026-0195.html), and
|
||||||
|
[RUSTSEC-2026-0186](https://rustsec.org/advisories/RUSTSEC-2026-0186.html).
|
||||||
|
|
||||||
|
No `npm audit` or `cargo audit` gate exists in CI.
|
||||||
|
|
||||||
|
## Browser/session and availability gaps
|
||||||
|
|
||||||
|
These do not currently prove a cross-org data read, but they are required before
|
||||||
|
exposing the service publicly:
|
||||||
|
|
||||||
|
- state-changing cookie-auth routes have no CSRF token or Origin/Sec-Fetch-Site
|
||||||
|
enforcement; SameSite=Lax is the only browser-side barrier;
|
||||||
|
- there is no session revocation/version store, so a stolen cookie remains valid
|
||||||
|
for up to seven days unless the global secret rotates;
|
||||||
|
- startup accepts any non-empty session secret rather than enforcing generated
|
||||||
|
entropy, and there is no key rotation scheme;
|
||||||
|
- no security-header policy is registered, and Fastify request timeout is zero;
|
||||||
|
- OAuth, login, and org APIs have no per-IP/user/tenant rate limit;
|
||||||
|
- Feishu file/post download count, response size, MIME, archive shape, and total
|
||||||
|
workspace quota are unbounded; uploads read entire files into memory.
|
||||||
|
|
||||||
|
The exact abuse quotas and file limits remain with
|
||||||
|
`Define initial abuse and capacity controls`; they must not be guessed here.
|
||||||
|
|
||||||
|
## Secret scan
|
||||||
|
|
||||||
|
All 377 tracked files were scanned for private-key blocks, common cloud/API-key
|
||||||
|
shapes, and non-empty assignments to the Hub's principal secret variables. No
|
||||||
|
private key or real credential was found. The only assignment hits were the
|
||||||
|
documented `<OpenRouter API key>` placeholder and local/test PostgreSQL URLs.
|
||||||
|
`hub/.env` is ignored and untracked.
|
||||||
|
|
||||||
|
This is positive repository hygiene, but it does not mitigate the runtime
|
||||||
|
environment leak into the agent process.
|
||||||
|
|
||||||
|
## Required security release evidence
|
||||||
|
|
||||||
|
Production readiness requires automated proof of all of the following:
|
||||||
|
|
||||||
|
1. An agent/Bash process cannot read service credentials, another project
|
||||||
|
workspace, repo-root files, or a symlink target outside its workspace.
|
||||||
|
2. Feishu context and file delivery validate the real target after resolution,
|
||||||
|
not only a caller-supplied lexical ID/path.
|
||||||
|
3. Every delayed action reauthorizes at execution and binds actor, chat,
|
||||||
|
organization, project, run, and pending action consistently.
|
||||||
|
4. ACTIVE Organization status is enforced in the shared authorization seam.
|
||||||
|
5. Feishu and provider credentials resolve by Organization, are encrypted at
|
||||||
|
rest with versioned keys, never enter logs/agent tools, and can rotate.
|
||||||
|
6. Cross-org HTTP and Prisma service tests remain green, while new negative
|
||||||
|
tests cover every finding above.
|
||||||
|
7. Production Node and Rust dependency audits pass with no ignored
|
||||||
|
vulnerability/unsound advisory unless a time-bounded, reviewed exception is
|
||||||
|
explicitly recorded.
|
||||||
|
8. Browser/session, request, and file-ingestion limits are verified against the
|
||||||
|
accepted production threat and capacity policy.
|
||||||
@@ -1,7 +1,7 @@
|
|||||||
# Audit tenant, authentication, and request security boundaries
|
# Audit tenant, authentication, and request security boundaries
|
||||||
|
|
||||||
Type: research
|
Type: research
|
||||||
Status: open
|
Status: resolved
|
||||||
|
|
||||||
## Question
|
## Question
|
||||||
|
|
||||||
@@ -10,3 +10,31 @@ Organization boundary, bypass the intended platform/org/project authorization
|
|||||||
seams, expose secrets, or accept unbounded hostile input in the initial
|
seams, expose secrets, or accept unbounded hostile input in the initial
|
||||||
production topology?
|
production topology?
|
||||||
|
|
||||||
|
## Answer
|
||||||
|
|
||||||
|
Yes. Basic org-admin HTTP/Prisma scoping is correctly enforced and its 28
|
||||||
|
targeted integration tests pass, but the agent inherits service secrets and can
|
||||||
|
read sibling files, MCP file/context tools accept real targets outside the run
|
||||||
|
objects, queued/card actions do not consistently bind or reauthorize their
|
||||||
|
target, non-ACTIVE tenants can still trigger agents, and Feishu/provider
|
||||||
|
credentials remain global instead of ADR-0021 org-scoped encrypted secrets.
|
||||||
|
Current Node and Rust dependency audits also fail on high/vulnerability/unsound
|
||||||
|
advisories.
|
||||||
|
|
||||||
|
The complete contract mapping, reproductions, positive evidence, advisory
|
||||||
|
inventory, and required release proofs are in the
|
||||||
|
[tenant, authentication, and request security audit](../assets/tenant-auth-security-audit.md).
|
||||||
|
|
||||||
|
The newly-clear implementation and decision frontier is:
|
||||||
|
|
||||||
|
- [Confine the agent runtime and protect service credentials](16-confine-agent-and-protect-credentials.md)
|
||||||
|
- [Close MCP context and file-delivery escape paths](17-close-mcp-data-egress-escapes.md)
|
||||||
|
- [Bind and reauthorize deferred and card actions](18-bind-and-reauthorize-deferred-actions.md)
|
||||||
|
- [Design the org-scoped secret and connection control plane](19-design-org-scoped-secret-control-plane.md)
|
||||||
|
- [Design the production browser and session boundary](20-design-production-web-session-boundary.md)
|
||||||
|
- [Clear dependency advisories and gate releases](21-clear-dependency-security-advisories.md)
|
||||||
|
- [Enforce Organization status at the shared authorization seam](22-enforce-organization-status.md)
|
||||||
|
|
||||||
|
Inbound Feishu file count/size/MIME and request-rate policy remain owned by
|
||||||
|
[Define initial abuse and capacity controls](05-define-abuse-capacity-controls.md)
|
||||||
|
rather than being guessed in this audit.
|
||||||
|
|||||||
@@ -2,7 +2,7 @@
|
|||||||
|
|
||||||
Type: task
|
Type: task
|
||||||
Status: open
|
Status: open
|
||||||
Blocked by: 01, 02, 03, 04, 05, 06, 07, 09, 10, 11, 12, 13, 14, 15
|
Blocked by: 01, 02, 03, 04, 05, 06, 07, 09, 10, 11, 12, 13, 14, 15, 16, 17, 18, 19, 20, 21, 22
|
||||||
|
|
||||||
## Question
|
## Question
|
||||||
|
|
||||||
|
|||||||
@@ -0,0 +1,12 @@
|
|||||||
|
# Confine the agent runtime and protect service credentials
|
||||||
|
|
||||||
|
Type: task
|
||||||
|
Status: open
|
||||||
|
|
||||||
|
## Question
|
||||||
|
|
||||||
|
Make the real Claude SDK/Bash execution surface uphold `AgentFileOp.Authorized`:
|
||||||
|
deny sibling tenant workspaces and service files, expose only the minimum safe
|
||||||
|
process environment, use SDK credential protection for secrets, and add an
|
||||||
|
actual Linux sandbox test proving another workspace and every non-provider
|
||||||
|
credential are unreadable while required tools still work.
|
||||||
@@ -0,0 +1,11 @@
|
|||||||
|
# Close MCP context and file-delivery escape paths
|
||||||
|
|
||||||
|
Type: task
|
||||||
|
Status: open
|
||||||
|
|
||||||
|
## Question
|
||||||
|
|
||||||
|
Restrict file delivery to a realpath/no-follow target inside the current project
|
||||||
|
workspace, remove Git-root access, make inbound Hub writes symlink-safe, and
|
||||||
|
validate every Feishu message/thread result against the run's bound chat before
|
||||||
|
returning data to the model.
|
||||||
@@ -0,0 +1,11 @@
|
|||||||
|
# Bind and reauthorize deferred and card actions
|
||||||
|
|
||||||
|
Type: task
|
||||||
|
Status: open
|
||||||
|
|
||||||
|
## Question
|
||||||
|
|
||||||
|
At execution time, reauthorize queued/batched triggers and validate one
|
||||||
|
actor/chat/organization/project/run-or-pending-action tuple for interrupt,
|
||||||
|
approval, and onboarding card actions so revoked permissions and mismatched
|
||||||
|
object IDs cannot affect another project.
|
||||||
+12
@@ -0,0 +1,12 @@
|
|||||||
|
# Design the org-scoped secret and connection control plane
|
||||||
|
|
||||||
|
Type: grilling
|
||||||
|
Status: open
|
||||||
|
|
||||||
|
## Question
|
||||||
|
|
||||||
|
Which key-management, envelope-encryption, rotation, connection identity, and
|
||||||
|
runtime resolver design will implement ADR-0021's org-scoped Feishu and model
|
||||||
|
provider credentials, support one customer-owned Feishu app per tenant, and
|
||||||
|
ensure plaintext credentials never reach business records, logs, or agent
|
||||||
|
tools?
|
||||||
+11
@@ -0,0 +1,11 @@
|
|||||||
|
# Design the production browser and session boundary
|
||||||
|
|
||||||
|
Type: grilling
|
||||||
|
Status: open
|
||||||
|
|
||||||
|
## Question
|
||||||
|
|
||||||
|
What same-origin/CSRF, proxy/TLS, security-header, session lifetime,
|
||||||
|
revocation, secret rotation, and request-timeout contract should the initial
|
||||||
|
org-admin web surface enforce, and which parts require server-side session state
|
||||||
|
rather than the current seven-day stateless cookie?
|
||||||
@@ -0,0 +1,11 @@
|
|||||||
|
# Clear dependency advisories and gate releases
|
||||||
|
|
||||||
|
Type: task
|
||||||
|
Status: open
|
||||||
|
|
||||||
|
## Question
|
||||||
|
|
||||||
|
Remove every current `npm audit --omit=dev` high finding and every `cargo audit`
|
||||||
|
vulnerability/unsound finding through tested upgrades, compatible overrides or
|
||||||
|
upstream fixes—not ignores—and add reproducible Node and Rust security-audit
|
||||||
|
gates to CI and the production release evidence.
|
||||||
@@ -0,0 +1,11 @@
|
|||||||
|
# Enforce Organization status at the shared authorization seam
|
||||||
|
|
||||||
|
Type: task
|
||||||
|
Status: open
|
||||||
|
|
||||||
|
## Question
|
||||||
|
|
||||||
|
Make non-ACTIVE Organizations fail closed for project authorization,
|
||||||
|
onboarding, queued execution, and Feishu triggers through one shared seam, with
|
||||||
|
negative tests proving a SUSPENDED or ARCHIVED tenant cannot create, bind,
|
||||||
|
trigger, resume, or mutate work while operator recovery remains explicit.
|
||||||
@@ -29,6 +29,7 @@ recovery procedures, and no known critical security or data-integrity gaps.
|
|||||||
## Decisions so far
|
## Decisions so far
|
||||||
|
|
||||||
- [Audit the clean-host deployment and rollback contract](issues/01-audit-clean-host-deployment.md) — keep the accepted single-host topology, but replace the incomplete in-place updater with a provisioned, immutable, readiness-gated and rollbackable release contract.
|
- [Audit the clean-host deployment and rollback contract](issues/01-audit-clean-host-deployment.md) — keep the accepted single-host topology, but replace the incomplete in-place updater with a provisioned, immutable, readiness-gated and rollbackable release contract.
|
||||||
|
- [Audit tenant, authentication, and request security boundaries](issues/02-audit-tenant-auth-security.md) — retain the working org-admin/application authorization core, but production is blocked on agent/MCP isolation, execution-time object binding, org-scoped encrypted credentials, tenant status, browser sessions, and clean dependency audits.
|
||||||
|
|
||||||
## Fog
|
## Fog
|
||||||
|
|
||||||
|
|||||||
Reference in New Issue
Block a user