Fail closed when an upload's owning book can't be resolved - #486
Conversation
Codex Review SummaryThis comment shows the latest Codex review activity on this pull request.
ℹ️ About Codex in GitHubYour team has set up Codex to review pull requests in this repo. Reviews are triggered when you
Codex reacts with 👀 while any review is running, comments if it has suggestions, and reacts with 👍 once all reviews finish with no findings. |
There was a problem hiding this comment.
Pull request overview
Fails closed when an upload’s owning book cannot be resolved, preventing unauthorized serving or public caching.
Changes:
- Denies access when
@bookisnil. - Adds regression coverage for orphaned attachments.
Tip
If you aren't ready for review, convert to a draft PR.
Click "Convert to draft" or run gh pr ready --undo.
Click "Ready for review" or run gh pr ready to reengage.
Reviewed changes
Copilot reviewed 2 out of 2 changed files in this pull request and generated 1 comment.
| File | Description |
|---|---|
app/controllers/action_text/markdown/uploads_controller.rb |
Makes attachment authorization fail closed. |
test/controllers/action_text/markdown/uploads_controller_test.rb |
Tests unresolved owning-book access. |
💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.
ensure_attachment_readable listed @book.nil? as the first allowed condition, so an attachment whose owning book couldn't be resolved short-circuited the check and was served — the opposite of the unpublished-uploads-stay-private invariant the guard exists to hold. A live attachment reaches this state when its leaf is severed without the destroy cascade that would otherwise purge it. Drop the nil branch and use safe navigation, so a nil book fails the check and 404s. Published/accessible books are unaffected.
b6e6fbb to
4af0220
Compare
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: b6e6fbbb8a
ℹ️ 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 "@codex address that feedback".
Matches the AGENTS.md controller/integration-test convention; no cross-host or absolute-URL behavior is under test here.
ActionText::Markdown::UploadsController#ensure_attachment_readablelisted@book.nil?as the first allowed condition:When
set_attachmentcan't resolve an owning book —@attachment.record.try(:record).try(:owning_book)returnsnil— thenil?branch short-circuits the guard and the file is served (and, for the show path, potentially cached publicly). That's the opposite of the invariant the guard exists to hold: an unpublished book's uploads are as private as the book itself.A live attachment reaches a nil owning book when its leaf is severed without the destroy cascade that would otherwise purge the attachment along with its markdown record. The guard should treat "no book to authorize against" as deny, not allow.
Fix
Drop the
nilbranch and use safe navigation, so a nil book fails both checks and 404s:Published and accessible books are unaffected — every existing serving test still passes.
Test
Adds a regression test: an attachment whose owning book can't be resolved is not served (404), asserted for an anonymous client against an unpublished book.