Skip to content

fix: retain user file ownership beyond session expiry (OPT-1251) - #21

Draft
Optale-ops wants to merge 1 commit into
mainfrom
security/opt-1251-durable-delete-owner
Draft

Optale-ops wants to merge 1 commit into
mainfrom
security/opt-1251-durable-delete-owner

Conversation

@Optale-ops

Copy link
Copy Markdown
Owner

OPT-1251: after the 24-hour Redis session expires, DELETE /v1/files/:sid/:fid was a permanent 403, so a File deleted in the Console left its bytes in MinIO.

Change: file-server uploads (API and sandbox output PUTs) store a hash of the server-owned user session namespace with the object, reusing the #20 object metadata. After session expiry, a user DELETE is authorized against that durable binding. Other users and tenants, conflicting live sessions and unbound objects are still refused. agent_run binding keeps precedence. Shared resource kinds get no fallback.

Known gap: objects uploaded before this change have no binding and stay refused after expiry (backfill decision pending with Thor; no ownership is invented).

Deploy: service API and file-server images; existing INTERNAL_SERVICE_TOKEN config; no new env, schema or service.

Verification so far: bun test src/agent-run.test.ts 36/36 (fake-store route harness: upload, Redis key removal, owner delete with bytes absent, other-user/tenant, legacy unbound and conflicting-session refusal); src/personal-parity.test.ts 1/1. Real Redis/MinIO proof and Astra medium review pending. Console side: Optale-no/console branch security/opt-1251-engine-file-delete.

@Optale-ops Optale-ops left a comment

Copy link
Copy Markdown
Owner Author

Choose a reason for hiding this comment

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

OPT-1251 fresh-context review #3

Reviewed b68e85e978f1612dd691a2b86f402f9b6e567c78...605d453e3a0ad019ed8674784b69ae4164907bd1 and the cross-process contract with Console PR #1104 at 982c17b3fa329429c5728695d404ed90a2f4e41a. The remote PR head matches the assigned commit; the three-dot diff is nonempty.

Findings

No material findings. Nothing is blocking. No hard violation or material judgement-call finding was substantiated by this static review.

Spec alignment

The change matches the supplied behavior contract. service/src/file-server.ts:267-273 derives the durable binding from the Redis session namespace and keeps a signed agent-run binding first. Both API uploads and sandbox output PUTs reach this writer. service/src/middleware/auth.ts:312-328 limits the new exception to user DELETE requests with an absent cache entry; a conflicting live entry remains a refusal. The expected value includes the server-derived tenant/user namespace, not the query's user ID. Shared kinds do not receive the exception, and reads still require the live session.

The existing owner-delete operation checks stored metadata before removing MinIO bytes. The API maps an explicit absent-object outcome to 404, but maps an unsupported old file-server operation to refusal, not deletion success. Existing agent-run target restrictions and binding precedence remain intact. Pre-change objects without a binding remain refused after expiry, as explicitly accepted. No out-of-scope hunk, new scheduler, schema, service, environment variable or permission model was identified.

Coverage and boundaries

Read all five changed-file diffs and the changed test cases, their setup, and relevant existing agent-run deletion regressions. Traced upload and batch registration, output-session creation, session-key resolution, API delete response handling, internal credentials, sandbox gateway PUT headers, and the Console route/auth/failure consumers. Applied auth/scope, input validation, cursor applicability, exact identity matching, cross-actor isolation, restart/replica persistence, deployment dependencies, liveness and scope checks. LSP references/definitions returned no useful results in these worktrees; text searches and direct source reads supplied caller coverage instead. Empty LSP results were not treated as absence.

Static-only review. No tests, builds, formatters or linters were run. The changed route test uses fake Redis/object storage and mirrors the file-server binding logic; it does not itself prove real MinIO metadata persistence or real sandbox output behavior. This review does not claim runtime or deployment verification.

Deployment order

Deploy the updated file-server before the updated service API, then deploy the Console build containing both its API and data-schema package changes. Existing CODEAPI_INTERNAL_SERVICE_TOKEN authentication is required. A mixed version refuses expired-session deletion and keeps the Console row rather than silently deleting metadata. Uploads reaching an old file-server still lack a user binding, so the documented legacy-object gap also covers that rollout interval. No backfill is introduced.

Verdict: PASS for static review at 605d453e3a0ad019ed8674784b69ae4164907bd1; nothing blocking.

@Optale-ops

Copy link
Copy Markdown
Owner Author

Review battery complete for OPT-1251 (GPT-6 Astra medium throughout). Engine Optale-ops/code-interpreter #21 at 605d453; Console Optale-no/console #1104 at 982c17b3fa329429c5728695d404ed90a2f4e41a.

  • Engine: passes 1, 2 and 3 found nothing.
  • Console: pass 2 found a P1 (the expiry sweep had no tenant identity, so in JWT mode it never sent an engine DELETE), a P2 (account deletion dropped the rows of files whose engine delete failed) and a starvation risk. Repair 982c17b3f: the sweep uses the stored deployment identity, account deletion keeps failed rows and marks them due (updatedAt unchanged), and a stable (expiredAt, _id) cursor pages past rows that keep failing. expiredAt never moves, since My Files shows it. Passes 1 and 2 re-reviewed and closed all three; pass 3 found nothing.
  • Real stack (Redis, MinIO, engine api and file_server, Console in JWT mode; artifacts /home/thor/optale-artifacts/security-engine-delete-1251-2026-10-06/): the owner deletes after session expiry with the bytes gone; another user, another tenant, a conflicting session and shared kinds are refused with the bytes kept; the sandbox output PUT route binds the owner; an object written by the current production image e9ad47c is refused after expiry (known gap). Console before the repair: the sweep failed with "Code API JWT auth requires tenant context". After the repair, the startup sweep deleted it; during a file_server outage the row and bytes are kept and the next sweep deletes them; account deletion keeps the failed file for the sweep; with limit 1 the cursor moves past a refused row to a valid one. A real My Files UI delete removes the row and the bytes.
  • Not exercised: full sandbox execution, and agent_run precedence on the real stack (covered by tests).
  • Deploy order: engine file_server, then engine service API, then the Console. No new env, schema or service. Objects written before the engine change stay refused after expiry; a backfill is pending Thor's decision.

Ready for Foundation release.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant