Authorize revision history and markdown uploads against current book access - #463
Conversation
Pages::EditsController resolves its leaf through PageLeafScoped, which looked up Current.user.leaves without the .active scope that every comparable lookup applies. Trashing a page records an edit that retains the old body, so the full content of a deleted page stayed readable through its history even though the page itself 404s on its own URL. Readers keep their existing access to the history of live pages. That is long-standing behavior and was assessed on its own terms: a reader can already read the page. It doesn't cover content an editor deleted, which is what this closes.
The uploads endpoint performed no authorization. It located a signed global id out of the request and attached the file, without consulting the Access table. The only gate was possession of the signed id that markdown_area renders into the page editor, minted with no purpose and no expiry, so it was a bearer capability that was never re-checked. A collaborator whose access had been revoked entirely kept writing files into that book's storage indefinitely. Uploads are now authorized against the book the record belongs to, at the time of the upload: no access row gives a 404, access below editor gives a 403. The signed id also carries a purpose and an expiry now, which bounds a leaked copy, though the authorization check rather than the token is what makes revocation take effect. Finding the book takes a small detour. Editing a page moves its leaf onto a copy and leaves the original held by the edit that records the revision, so a superseded page has no leaf of its own and has to reach its book through that edit.
/u/*slug served any attachment to anyone holding the URL, with no session required, and cached the response publicly for a year. Uploads belonging to an unpublished book are as private as the book, so this was a way to read them without an access row, and the public cache directive put them into shared caches on the way. Attachments of published books are served and cached as before. Otherwise the request now needs an access row on the owning book, and the response is marked private.
The only link to this screen is in the page editor, which is editor-gated, so the interface has always expressed this restriction and the controller simply never enforced it. No reader reaches revision history through the UI; it takes a hand-constructed URL. Revision history shows text an editor removed from a page. That is content a reader can't otherwise see even when the page itself is still readable, so 'a reader can already read the page' doesn't fully cover it. Git history shows no decision to allow reader access here: the Current.user.leaves lookup arrived in a URL-stability refactor, and the one permissions commit around it was about who may write.
![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: b4e74b3f53
ℹ️ 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".
| if @book&.published? | ||
| expires_in 1.year, public: true | ||
| else | ||
| expires_in 5.minutes, public: false |
There was a problem hiding this comment.
For unpublished books in the inspected production configuration, this only marks the /u/... redirect private; config/environments/production.rb:28 selects the :local service, config/storage.yml:5-8 configures that Disk service as public: true, and config/initializers/active_storage.rb:1-4 publicly caches the final Disk response for a year. Consequently, once an authorized reader follows this redirect, its permanent unauthenticated target can be reused or shared after access is revoked, bypassing the new check. Private uploads need authenticated/proxied delivery of the actual bytes rather than only a private redirect.
Useful? React with 👍 / 👎.
| markdown.uploads.attach fixture_file_upload("reading.webp", "image/webp") | ||
| test "a signed id minted for some other purpose can't be used to upload" do | ||
| assert_no_changes -> { ActiveStorage::Attachment.count } do | ||
| post action_text_markdown_uploads_url, params: { |
There was a problem hiding this comment.
The newly added requests here and throughout both changed controller test files use _url helpers even though none exercises multiple hosts or requires an absolute URL. Replace them with the corresponding _path helpers as required for controller/integration tests.
AGENTS.md reference: AGENTS.md:L8-L9
Useful? React with 👍 / 👎.
Four fixes from a batch of externally reported findings. Each commit stands on its own and carries a test that fails on
main.Keep trashed pages out of revision history
The page revision history lookup omitted the
.activescope applied elsewhere. Deleting a page records a revision that retains its contents, so the body of a deleted page stayed readable through the history endpoint even though the page itself returned 404.This restores pre-refactor behavior rather than changing a design decision. Until 2024-06-12
Leafcarrieddefault_scope { where.not(status: :trashed) };2459fb0removed it for an unrelated reason (dependent: :destroy) and hand-added.activeto six call sites. The edits controller was not one of them because it keyed offPageat the time — anddc30e73rewrote it into a genuineleaveslookup the very next day, without the scope.Authorize markdown uploads against current book access
The markdown upload endpoint authorized writes solely by possession of a signed identifier rendered into the page editor, minted with no purpose and no expiry and never re-validated. Someone whose access to a book had been revoked kept the ability to write files into that book's storage indefinitely. Uploads are now authorized against current access, and the signed identifier carries a purpose and an expiry.
Gate uploaded attachments on access to their book
Uploaded attachments were served to any client holding the URL and marked publicly cacheable for a year, regardless of whether the owning book was published. Attachments of unpublished books now require access to the book and are no longer publicly cacheable.
Restrict revision history to editors
Hardening rather than a defect fix. The only link to revision history lives in the editor-gated page editor, so no reader reaches this screen through the interface — the controller now matches what the interface already expresses. It also covers a gap the current-page-is-readable argument does not: history shows text edited out of a page that is still live.
Tests:
bin/rails test193 runs / 601 assertions / 0 failures;bin/rails test:system2 runs / 8 assertions / 0 failures;bin/rubocopclean on all changed files. Every new test was confirmed to fail onmainfirst.