fix(permissions): hide unreadable pages from menu and breadcrumb - #4138
Open
tvdeyen wants to merge 1 commit into
Open
fix(permissions): hide unreadable pages from menu and breadcrumb#4138tvdeyen wants to merge 1 commit into
tvdeyen wants to merge 1 commit into
Conversation
render_breadcrumb listed every published ancestor and the menu partials rendered every child node, so a restricted page a user may not read still leaked its name and url through navigation. Both now filter by the current user's read permission, using readable_by? so the check stays a column read with no extra query and honors permitted_roles. render_breadcrumb drops ancestors the current user cannot read. A new Alchemy::Node#readable_children returns only children whose page is readable (external nodes without a page are always kept), and the generated menu partials render through it and add the current user to their cache key, so a filtered menu is never served to a different user. Because those partials are generated into the host app, an 8.4 upgrade todo tells existing apps to apply the same change to their own menu partials.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
What is this pull request for?
Follow-up to the security review of #4112. Page read access is enforced in the authorization layer, but two frontend helpers rendered navigation without consulting it, so a restricted page (now including one limited to certain roles) still leaked its name and url:
render_breadcrumblisted every published ancestor of the current page.render_menurendered every child node, and the menu fragment cache key did not include the user.Both now filter by the current user's read permission via
Page#readable_by?, which keeps the check a column read (no extra query) and automatically honorspermitted_roles.Notable changes
render_breadcrumbdrops ancestors the current user cannot read.Alchemy::Node#readable_children(user)returns only children whose page is readable (external nodes without a page are always kept). The generated menu partials (wrapper/node, erb/haml/slim) render through it and add the current user to theircachekey, so a filtered menu is never served to a different user.todo(bin/rails alchemy:upgrade) tells maintainers to apply the same change (or regenerate withbin/rails g alchemy:menus). The dummy app's partials are updated so the behavior is covered by specs.Specs cover: breadcrumb hides a restricted ancestor from a guest but shows it to a permitted member; a menu node to a restricted page is hidden from a guest and shown to a permitted member;
Node#readable_childrenfiltering; and the upgrade todo.Note
Stacked on #4112 — this depends on
Page#readable_by?from the permitted-roles branch, so its base ispermitted-roles-for-restricted-pages. Merge after #4112; GitHub will retarget the base tomainonce #4112 lands.Checklist