Skip to content

fix: escape literal % in DocShare folder LIKE condition - #138

Merged
agritheory merged 1 commit into
version-15from
fix/percent_escape
Jul 29, 2026
Merged

fix: escape literal % in DocShare folder LIKE condition#138
agritheory merged 1 commit into
version-15from
fix/percent_escape

Conversation

@lauty95

@lauty95 lauty95 commented Jul 29, 2026

Copy link
Copy Markdown
Collaborator

I ran some tests: everything works fine on version-15, but on version-16 the tests in test_file_permissions.py only pass 1/4:
Error:

query = b"SELECT `name` FROM `tabFile` WHERE `folder`=%(param1)s AND ((((( `tabFile`.`owner` = 'pat.viewer@testcompany.test' )...ONCAT(shared_folder.name, '/', '%')\n\t\t)\n\t))))))) OR (`name` IN (%(param2)s,%(param3)s))) ORDER BY `creation` DESC"
args = {b'param1': b"'Home'", b'param2': b"'60810aa065'", b'param3': b"'Home/DocShare Inherit Folder'"}

    def _mogrify(self, query, args=None):
        """Return query after binding args."""
        db = self._get_db()
    
        if isinstance(query, str):
            query = query.encode(db.encoding)
    
        if args is not None:
            if isinstance(args, dict):
                nargs = {}
                for key, item in args.items():
                    if isinstance(key, str):
                        key = key.encode(db.encoding)
                    nargs[key] = db.literal(item)
                args = nargs
            else:
                args = tuple(map(db.literal, args))
            try:
>               query = query % args # This is where the problem is

The only test that passes doesn't use frappe.get_list(), but frappe.has_permission() instead (test: test_shared_user_has_read_permission_on_file)

@agritheory once mentioned that Frappe's get_list method changes between versions. Looking into it in detail:

version-15:

	import frappe.model.db_query

	return frappe.model.db_query.DatabaseQuery(doctype).execute(*args, **kwargs)

version-16:

	import frappe.model.qb_query

	return frappe.model.qb_query.DatabaseQuery(doctype).execute(*args, **kwargs)

Reading the error traceback:

../frappe/frappe/__init__.py:1380: in get_list
    return frappe.model.qb_query.DatabaseQuery(doctype).execute(*args, **kwargs)
../frappe/frappe/model/qb_query.py:210: in execute
    result = query.run(debug=debug, as_dict=True, pluck=pluck)
../frappe/frappe/query_builder/utils.py:133: in execute_query
    result = frappe.local.db.sql(query, params, *args, **kwargs)  # nosemgrep
../frappe/frappe/database/database.py:272: in sql
    self.execute_query(query, values)
../frappe/frappe/database/database.py:372: in execute_query
    return self._cursor.execute(query, values)
../../env/lib/python3.14/site-packages/MySQLdb/cursors.py:176: in execute
    mogrified_query = self._mogrify(query, args)

The problem happens when _mogrify() receives arguments together with a query where we pass a %, because it ends up doing something like:

"WHERE folder=%(param1)s AND (folder LIKE CONCAT(x, '/', '%'))" % {"param1": "Home"}
TypeError: not enough arguments for format string

That's why the fix that works for both version-15 and version-16 is:

In [1]: "WHERE folder=%(param1)s AND (folder LIKE CONCAT(x, '/', '%%'))" % {"param1": "Home"}
Out[1]: "WHERE folder=Home AND (folder LIKE CONCAT(x, '/', '%'))"

With this change, the tests pass on both version-15 and version-16.

@lauty95
lauty95 requested a review from agritheory July 29, 2026 12:09
@github-actions

Copy link
Copy Markdown

Draft Changelog Entry

Changelog Entry

Fixed an issue where file permission queries were failing in certain system configurations. The DocShare folder permission check was using a literal percent character (%) in a database query condition, which caused errors when the query builder attempted to process parameters. The percent character is now properly escaped to prevent conflicts with the query parameter formatting system.

This changelog entry was automatically generated by the Changelog Generator Action.

@github-actions

Copy link
Copy Markdown

Coverage

Coverage Report
FileStmtsMissCoverMissing
__init__.py17571%24–25, 31–33
commands.py35350%4–6, 9–11, 17–20, 23, 26–27, 49–51, 53–54, 56, 65, 68–72, 75–76, 85–87, 89–90, 92, 94, 97
hooks.py160100% 
install.py440%4, 6, 9–10
migration.py19611840%25, 46, 68–69, 88–89, 96, 115, 118–120, 123–125, 158–159, 161–162, 166–168, 171–172, 187, 196–197, 199–203, 205–210, 212, 219–221, 223, 225–226, 228, 235–236, 238–242, 244–247, 249, 254–257, 259–268, 270–279, 281–282, 284–285, 287–288, 290–294, 298–301, 305, 307–308, 310, 312–314, 317–318, 320–321, 323–329, 331–332, 334, 336
permissions.py20385%14, 19, 21
system_packages.py34340%4, 11, 13–15, 17, 20–21, 24–27, 37–38, 48, 50–55, 57–59, 61–62, 66–69, 71–74
cloud_storage
   __init__.py00100% 
cloud_storage/doctype
   __init__.py00100% 
cloud_storage/doctype/file_association
   __init__.py00100% 
   file_association.py30100% 
cloud_storage/doctype/file_version
   __init__.py00100% 
   file_version.py30100% 
cloud_storage/overrides
   file.py41713069%66, 76, 96, 109, 126, 140, 148–149, 162, 166–167, 179, 194, 199, 206, 221, 279–288, 290–298, 309, 314–315, 323–325, 327, 332, 338, 340, 356, 360, 365, 373–374, 377, 380, 409, 454, 457, 496, 502, 508, 514, 520, 526, 536, 538–540, 544, 552–554, 556, 574, 589–593, 599–600, 602, 625–629, 641–642, 691, 694–703, 710–711, 713–715, 718–719, 724–726, 728, 733–736, 741, 743, 755–756, 758, 763–767, 769, 780–786
config
   __init__.py00100% 
   desktop.py330%4, 7–8
   docs.py660%4, 6–8, 12–13
tests
   conftest.py49198%55
   fixtures.py30100% 
   test_file.py188398%88, 173, 256
   test_file_permissions.py390100% 
   test_file_preview.py630100% 
   test_local_storage.py610100% 
TOTAL115734270% 

@agritheory
agritheory merged commit 432501f into version-15 Jul 29, 2026
7 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants