Skip to content

Harden processor content hash access control - #15

Merged
2witstudios merged 5 commits into
masterfrom
codex/secure-processor-service-endpoints
Sep 29, 2025
Merged

2witstudios merged 5 commits into
masterfrom
codex/secure-processor-service-endpoints

Conversation

@2witstudios

Copy link
Copy Markdown
Owner

Summary

  • validate content hashes within the processor content store before touching the filesystem and expose sanitized cache metadata helpers
  • enforce hash format checks plus tenant authorization on file serving endpoints while returning scrubbed preset metadata
  • reject ingestion and optimization requests for pages outside the caller tenant and surface consistent invalid-hash errors

Testing

  • pnpm --filter @pagespace/processor typecheck
  • pnpm lint

https://chatgpt.com/codex/tasks/task_e_68d5e1ad0c008320b2bad3a5bfa44320

@chatgpt-codex-connector chatgpt-codex-connector Bot left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

💡 Codex Review

Here are some automated review suggestions for this pull request.

ℹ️ About Codex in GitHub

Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you

  • Open a pull request for review
  • Mark a draft as ready
  • Comment "@codex review".

If Codex has suggestions, it will comment; otherwise it will react with 👍.

Codex can also answer questions or update the PR. Try commenting

Comment on lines +125 to +129
contentHash,
tenantId,
service: auth.service
});
return res.status(403).json({ error: 'File belongs to a different tenant' });

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

[P1] Clean up temp file when cross-tenant upload is rejected

In the early exit for deduped uploads that belong to another tenant (upload.ts), the handler logs and returns a 403 without removing the temporary file created by Multer. Because tempFilePath is set earlier and the subsequent cleanup logic only runs after this block, every unauthorized dedupe attempt leaves the uploaded file under the temp directory, allowing repeated requests to fill disk space. Delete the temp file before returning the error so rejected uploads don’t accumulate on disk.

Useful? React with 👍 / 👎.

@2witstudios
2witstudios merged commit 2b492f5 into master Sep 29, 2025
@2witstudios
2witstudios deleted the codex/secure-processor-service-endpoints branch September 29, 2025 00:15
2witstudios added a commit that referenced this pull request Jul 29, 2026
…ty, error UX, reconcile resilience

Six findings from the full-branch review. (#1 SSRF, #2 storage metering and #7
concurrency quota were already fixed in the two prior commits; #3's cron suite
now collects and passes 5 tests, so it was fixed by a later phase than the
review sampled.)

#4/#15 — prompt regression on default config. `buildAgentAwarenessPrompt`
unconditionally told the model to delegate with `spawn_session`, but session
tools only exist when CODE_EXECUTION_ENABLED is on (default off), so the
assistant confidently called a tool it did not have and delegation silently
broke. The delegation sentence is now gated on a `canDelegate` flag that both
call sites derive from `isCodeExecutionEnabled()`; without it the section still
lists the agents, it just stops naming a tool that isn't there.

#13/#14 — git argv safety. `git_add` spliced paths straight into argv with no
`--`, unlike its siblings `git status`/`git diff`: a path named `-p` was read as
a flag (interactive add). It now separates, and only when there are paths, so no
bare trailing `--`. `git_reset.ref` and `git_remote_add.name` gained the
`validateFlagSafe` guard that structurally identical sibling fields already
apply.

#10/#11 — infinite spinner on a failed agent load. `AgentView` guarded on
`agentLoading || !agent`, so once SWR gave up retrying, `isLoading` went false,
`agent` stayed null, and the user watched a spinner that would never resolve
with no error text and no escape but a reload. Loading and failure are now
distinct states: the failure surfaces the server's own message and a Try again
button, backed by a new `retry` from `useResolvedAgent`.

#6 — one failing candidate query no longer parks the other pass. The orphan
reconcile listed the reclaim outbox and the teardown-intent rows under
`Promise.all`, so either failing dropped both. Now `allSettled` with per-source
error logs: a degraded query costs its own candidates, not every reclaim, and
those are billing VMs nobody is using.

#8 — the leak signal is no longer silent. When a confirmed-unreferenced Sprite
fails BOTH its kill and its reclaim-outbox insert, nothing in the system knows
that VM exists — no row points at it, so no trigger and no cross-check will find
it. That path swallowed its error, making the one path built to catch a
permanently leaked VM the one path with no signal. It now logs loudly with the
sandbox id and both failure reasons.

#5 — the destructive teardown binding gets tests. 10 cases over `killSprite`
(confirmed kill, replaced-name-as-success, genuine failure, unpinned instance),
`markSessionTornDown` (CAS win/loss), and `listOrphanCandidates` (both sources,
each single-source failure, cap + backlog).

Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_017V3eqwRX5cFy3Tdu2Syoro
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant