fix:validate the route to ensure wiki is not accepted#638
Open
wreckage0907 wants to merge 2 commits into
Open
Conversation
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Organization UI Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (2)
🚧 Files skipped from review as they are similar to previous changes (1)
WalkthroughThis pull request adds route-namespace validation to the WikiSpace doctype to prevent creation or updates using routes reserved by the wiki editor. A new 🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
wiki/wiki/doctype/wiki_space/wiki_space.py (1)
243-248:⚠️ Potential issue | 🟠 Major | ⚡ Quick winValidate reserved routes before enqueueing the clone job.
Currently,
clone_wiki_space_in_backgroundvalidates empty/duplicate routes but not reserved routes. The validation will eventually happen when_create_space_copycallsnew_space.insert()(line 314), but that occurs asynchronously in the background job. If a user attempts to clone to a reserved route, they won't receive immediate feedback and the job will fail silently (or in logs), leading to a poor user experience.🛡️ Proposed fix to add early validation
new_route = (new_space_route or "").strip().strip("/") if not new_route: frappe.throw(_("Route cannot be empty")) + if is_reserved_route(new_route): + frappe.throw( + _( + "Route cannot be '{0}' or start with '{0}/' — that path is reserved for the " + "Wiki editor. Use a different route, e.g. 'docs'." + ).format(RESERVED_ROUTE), + frappe.ValidationError + ) + if new_route == self.route: frappe.throw(_("New route is the same as current route"))🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@wiki/wiki/doctype/wiki_space/wiki_space.py` around lines 243 - 248, The clone_wiki_space_in_background function currently only checks empty/duplicate routes but defers reserved-route validation to the async _create_space_copy (which calls new_space.insert()), so add an early synchronous reserved-route check before enqueueing: reuse the same route validation logic used by _create_space_copy/new_space.insert() (or call the shared route validation helper if one exists) and call frappe.throw with a clear message if the route is reserved; ensure this check references clone_wiki_space_in_background, _create_space_copy, and new_space.insert() so the validation happens immediately and prevents enqueueing a job for a reserved route.
🧹 Nitpick comments (3)
wiki/wiki/doctype/wiki_space/test_wiki_space.py (1)
311-337: ⚡ Quick winConsider adding test coverage for the
update_routes()method.The current tests verify that reserved routes are rejected during insert, which is good. However, the
update_routes()method also validates reserved routes (wiki_space.py:174-179) and should be tested to ensure that existing spaces cannot be updated to use reserved routes.🧪 Suggested test case
def test_update_routes_rejects_reserved_route(self): # Create a space with a valid route space = frappe.get_doc( { "doctype": "Wiki Space", "space_name": f"Update Test {frappe.generate_hash(length=6)}", "route": f"docs-{frappe.generate_hash(length=6)}", } ).insert() # Attempt to update to a reserved route should fail with self.assertRaises(frappe.ValidationError): space.update_routes("wiki") with self.assertRaises(frappe.ValidationError): space.update_routes("wiki/updated")🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@wiki/wiki/doctype/wiki_space/test_wiki_space.py` around lines 311 - 337, Add a new unit test in TestWikiSpaceRouteValidation that asserts Wiki Space.update_routes() rejects reserved routes: create a Wiki Space with a valid route (use frappe.generate_hash like other tests), then call the instance method update_routes("wiki") and update_routes("wiki/updated") inside with self.assertRaises(frappe.ValidationError) blocks to ensure the validation logic in update_routes() is covered; reference the TestWikiSpaceRouteValidation class and the update_routes() method when adding the test.wiki/wiki/doctype/wiki_space/wiki_space.py (2)
174-179: ⚡ Quick winPass exception type explicitly and align error message with
validate_route_namespace().Two issues:
- Missing explicit
frappe.ValidationError(same as Line 69-76)- The error message omits the helpful suggestion "Use a different route, e.g. 'docs'." that appears in
validate_route_namespace(). Consistent messaging improves UX.♻️ Proposed fix
if is_reserved_route(new_route): frappe.throw( _( - "Route cannot be '{0}' or start with '{0}/' — that path is reserved for the Wiki editor." - ).format(RESERVED_ROUTE) + "Route cannot be '{0}' or start with '{0}/' — that path is reserved for the " + "Wiki editor. Use a different route, e.g. 'docs'." + ).format(RESERVED_ROUTE), + frappe.ValidationError )🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@wiki/wiki/doctype/wiki_space/wiki_space.py` around lines 174 - 179, Replace the generic frappe.throw call in the is_reserved_route check with an explicit frappe.ValidationError (same pattern used in validate_route_namespace) and update the error message to match validate_route_namespace() by adding the suggestion text "Use a different route, e.g. 'docs'."; specifically modify the block that calls is_reserved_route(new_route) in wiki_space.py to throw frappe.ValidationError and include the full message: "Route cannot be '{0}' or start with '{0}/' — that path is reserved for the Wiki editor. Use a different route, e.g. 'docs'." so messaging and exception type are consistent with validate_route_namespace().
69-76: ⚡ Quick winPass exception type explicitly to
frappe.throw()for consistency.While
frappe.throw()defaults tofrappe.ValidationError, the codebase pattern is to pass it explicitly for clarity. As per coding guidelines, wiki/frappe_wiki/doctype/wiki_change_request/wiki_change_request.py:777-807 showsfrappe.throw(_("..."), frappe.ValidationError).♻️ Proposed fix
def validate_route_namespace(self): if is_reserved_route(self.route): frappe.throw( _( "Route cannot be '{0}' or start with '{0}/' — that path is reserved for the " "Wiki editor. Use a different route, e.g. 'docs'." - ).format(RESERVED_ROUTE) + ).format(RESERVED_ROUTE), + frappe.ValidationError )🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@wiki/wiki/doctype/wiki_space/wiki_space.py` around lines 69 - 76, The validate_route_namespace method currently calls frappe.throw without an explicit exception type; update the call in validate_route_namespace (inside class/method validate_route_namespace) to pass frappe.ValidationError as the second argument so it matches project convention (use the same pattern as wiki_change_request's throw calls), keeping the existing formatted message that references RESERVED_ROUTE and is_reserved_route.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Outside diff comments:
In `@wiki/wiki/doctype/wiki_space/wiki_space.py`:
- Around line 243-248: The clone_wiki_space_in_background function currently
only checks empty/duplicate routes but defers reserved-route validation to the
async _create_space_copy (which calls new_space.insert()), so add an early
synchronous reserved-route check before enqueueing: reuse the same route
validation logic used by _create_space_copy/new_space.insert() (or call the
shared route validation helper if one exists) and call frappe.throw with a clear
message if the route is reserved; ensure this check references
clone_wiki_space_in_background, _create_space_copy, and new_space.insert() so
the validation happens immediately and prevents enqueueing a job for a reserved
route.
---
Nitpick comments:
In `@wiki/wiki/doctype/wiki_space/test_wiki_space.py`:
- Around line 311-337: Add a new unit test in TestWikiSpaceRouteValidation that
asserts Wiki Space.update_routes() rejects reserved routes: create a Wiki Space
with a valid route (use frappe.generate_hash like other tests), then call the
instance method update_routes("wiki") and update_routes("wiki/updated") inside
with self.assertRaises(frappe.ValidationError) blocks to ensure the validation
logic in update_routes() is covered; reference the TestWikiSpaceRouteValidation
class and the update_routes() method when adding the test.
In `@wiki/wiki/doctype/wiki_space/wiki_space.py`:
- Around line 174-179: Replace the generic frappe.throw call in the
is_reserved_route check with an explicit frappe.ValidationError (same pattern
used in validate_route_namespace) and update the error message to match
validate_route_namespace() by adding the suggestion text "Use a different route,
e.g. 'docs'."; specifically modify the block that calls
is_reserved_route(new_route) in wiki_space.py to throw frappe.ValidationError
and include the full message: "Route cannot be '{0}' or start with '{0}/' — that
path is reserved for the Wiki editor. Use a different route, e.g. 'docs'." so
messaging and exception type are consistent with validate_route_namespace().
- Around line 69-76: The validate_route_namespace method currently calls
frappe.throw without an explicit exception type; update the call in
validate_route_namespace (inside class/method validate_route_namespace) to pass
frappe.ValidationError as the second argument so it matches project convention
(use the same pattern as wiki_change_request's throw calls), keeping the
existing formatted message that references RESERVED_ROUTE and is_reserved_route.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 80e1fd6e-f96e-4519-8eba-e9d6c66fc4fd
📒 Files selected for processing (2)
wiki/wiki/doctype/wiki_space/test_wiki_space.pywiki/wiki/doctype/wiki_space/wiki_space.py
…rmalise throw calls
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.
Description
This pull request introduces validation to prevent the creation or update of
Wiki Spacedocuments with routes that conflict with the reserved/wikinamespace, which is used by the Wiki editor SPA. The changes ensure that routes cannot be set towikior any subpath underwiki/, both when creating, updating, or cloning a wiki space. Comprehensive tests are also added to verify this behavior.Route validation enhancements:
is_reserved_routefunction and aRESERVED_ROUTEconstant to centralize logic for detecting reserved routes (wikiand its subpaths) inwiki_space.py.validate,update_routes, andclone_wiki_space_in_backgroundmethods in theWiki Spacedoctype to reject reserved routes by raising afrappe.ValidationErrorwith a clear message. [1] [2] [3]Testing improvements:
TestWikiSpaceRouteValidationto ensure that reserved routes are correctly rejected during creation and updates, and that valid routes are still allowed.Adds validation so a Wiki Space can't be created or updated with a route of
wikior one starting withwiki/. That path is reserved for internal use (the editor SPA), so such spaces were unreachable and rendered a blank page. Users now get a clear error and are pointed to a valid route (e.g.docs).Relevant technical information
is_reserved_route()helper added;validate()andupdate_routes()reject reserved routes.test_wiki_space.py.Screenshot
Fixes #637
Summary by CodeRabbit
New Features
Tests