adam/Piwigo
1
0 Fork 0
mirror of https://github.com/Piwigo/Piwigo.git synced 2026-10-30 21:52:00 +01:00
Code Issues 657 Packages Projects Releases Wiki Activity

escaping 'groups' and other reserved words by default in pwg_query leads to various bugs #1166

New Issue
Closed
opened 2026-10-05 00:33:59 +02:00 by adam · 7 comments
adam commented 2026-10-05 00:33:59 +02:00
Owner
Copy link
Copy source

pwg_query is escaping 'groups' and 'rank' in the query, without taking into account if the word is not already in a quoted part of the query (in strings in particular).
This bug already shows up in #1292
I've also been hit by this bug with the Ldap_Login plugin.
This plugin calls mass_updates that calls mass_insert that calls pwg_query
Managing ldap config, it tries for example to insert the following data (from mariadb logs):

2021-02-15T20:53:01.691167Z	  265 Query	INSERT  INTO `piwigo_ldap_login_config_1613422381676754`
  (`param`,`value`)
  VALUES
[...]
  , ('ld_group_basedn','cn=`groups`,dc=domain,dc=tld')
[...]

You see the problem: the group in the data must not be quoted here.
I'm under the impression that the decision of escaping word in query without parsing it in detail (to avoid to quote in quoted string) is wrong.

What do you suggest? I'm thinking of various fixes:

  1. changing the default behavior of pwg_query to avoid quoting by default
  2. adding the $escape_reserved_words to all core functions that calls pwg_query (at least mass_updates and mass_insert for the Ldap_Login plugin (seems intrusive to me)
  3. fixing the quoting in pwg_query to avoid to modify already quoted strings (probably difficult to implement)
  4. removing the whole quoting stuff in pwg_query and fixing code that does not quote correctly its data/column names/etc (seems the good way for me)

Regards
Vincent

`pwg_query` is escaping 'groups' and 'rank' in the query, without taking into account if the word is not already in a quoted part of the query (in strings in particular). This bug already shows up in #1292 I've also been hit by this bug with the Ldap_Login plugin. This plugin calls `mass_updates` that calls `mass_insert` that calls `pwg_query` Managing ldap config, it tries for example to insert the following data (from mariadb logs): ``` 2021-02-15T20:53:01.691167Z 265 Query INSERT INTO `piwigo_ldap_login_config_1613422381676754` (`param`,`value`) VALUES [...] , ('ld_group_basedn','cn=`groups`,dc=domain,dc=tld') [...] ``` You see the problem: the `group` in the data must not be quoted here. I'm under the impression that the decision of escaping word in query without parsing it in detail (to avoid to quote in quoted string) is wrong. What do you suggest? I'm thinking of various fixes: 1) changing the default behavior of `pwg_query` to avoid quoting by default 2) adding the `$escape_reserved_words` to all core functions that calls `pwg_query` (at least `mass_updates` and `mass_insert` for the Ldap_Login plugin (seems intrusive to me) 3) fixing the quoting in `pwg_query` to avoid to modify already quoted strings (probably difficult to implement) 4) removing the whole quoting stuff in `pwg_query` and fixing code that does not quote correctly its data/column names/etc (seems the good way for me) Regards Vincent <!-- github-sync-id:808827497 -->
adam closed this issue 2026-10-05 00:33:59 +02:00
adam commented 2026-10-05 00:34:00 +02:00
Author
Owner
Copy link
Copy source

Note: modifying safe_unserialize as suggested in #1292 seems wrong to me. In this case, we wont be able to insert data such as ' groups and groups ' => currently, both will be quoted. Modifying safe_unserialize will lead to both being unquoted...

Note: modifying `safe_unserialize` as suggested in #1292 seems wrong to me. In this case, we wont be able to insert data such as ' groups and `groups` ' => currently, both will be quoted. Modifying `safe_unserialize` will lead to both being unquoted... <!-- github-comment-id:779456061 -->
adam commented 2026-10-05 00:34:01 +02:00
Author
Owner
Copy link
Copy source

See also the still pending plugins PRs:
https://github.com/Piwigo/AMenuManager/pull/7
https://github.com/Piwigo/GrumPluginClasses/pull/3
https://github.com/Piwigo/PWG_Stuffs/pull/12

Fixing the actual now wrong code places to escape groups column names would be preferable over hitting all with a big club and introducing new problems.

See also the still pending plugins PRs: https://github.com/Piwigo/AMenuManager/pull/7 https://github.com/Piwigo/GrumPluginClasses/pull/3 https://github.com/Piwigo/PWG_Stuffs/pull/12 Fixing the actual now wrong code places to escape `groups` column names would be preferable over hitting all with a big club and introducing new problems. <!-- github-comment-id:782596813 -->
adam commented 2026-10-05 00:34:02 +02:00
Author
Owner
Copy link
Copy source

I agree solution 4 is the best one. The "short term" solution I implemented brings too much unexpected side effects.

I agree solution 4 is the best one. The "short term" solution I implemented brings too much unexpected side effects. <!-- github-comment-id:782703122 -->
adam commented 2026-10-05 00:34:03 +02:00
Author
Owner
Copy link
Copy source

If it can help a bit, here is the patch I did to remove this quoting in my development version. Not sure I found all the places (in particular, I did not look for 'groups'), but it is a start.
patch.txt

If it can help a bit, here is the patch I did to remove this quoting in my development version. Not sure I found all the places (in particular, I did not look for 'groups'), but it is a start. [patch.txt](https://github.com/Piwigo/Piwigo/files/6018339/patch.txt) <!-- github-comment-id:782911957 -->
adam commented 2026-10-05 00:34:03 +02:00
Author
Owner
Copy link
Copy source

@vdanjean I would like to keep the solution 4 for Piwigo 12 (I don't want plugins relying on this auto-escape to be broken in Piwigo 11.4.0). Can you find a way to avoid the automatic escaping in Ldap plugin?

@vdanjean I would like to keep the solution 4 for Piwigo 12 (I don't want plugins relying on this auto-escape to be broken in Piwigo 11.4.0). Can you find a way to avoid the automatic escaping in Ldap plugin? <!-- github-comment-id:792787927 -->
adam commented 2026-10-05 00:34:04 +02:00
Author
Owner
Copy link
Copy source

Similar issue with pwg_session_* :

with default $conf['session_save_handler'] = 'db, if the session contains "groups" (*), session can not be read:

session_start(): Failed to decode session object. Session has been destroyed

Suggested fix: pwg_query($query, false) in function pwg_session_write

Workaround: force the default PHP session handle with $conf['session_save_handler'] = null;

(*) example : $_SESSION['foo'] = 'groups'

Similar issue with pwg_session_* : with default `$conf['session_save_handler'] = 'db`, if the session contains "groups" (*), session can not be read: ``` session_start(): Failed to decode session object. Session has been destroyed ``` Suggested fix: `pwg_query($query, false)` in function `pwg_session_write` Workaround: force the default PHP session handle with `$conf['session_save_handler'] = null;` (*) example : `$_SESSION['foo'] = 'groups'` <!-- github-comment-id:806534946 -->
adam commented 2026-10-05 00:34:05 +02:00
Author
Owner
Copy link
Copy source

Same problem if a physical to be synced directory contains a groups name, see https://piwigo.org/forum/viewtopic.php?id=31183

Same problem if a physical to be synced directory contains a `groups` name, see https://piwigo.org/forum/viewtopic.php?id=31183 <!-- github-comment-id:817139078 -->
Sign in to join this conversation.
No Branch/Tag Specified
Labels
No items
No labels
Milestone
No items
No Milestone
Projects
Clear projects
No projects
Assignees
Clear assignees
adam (Adam Wojcieszonek)
No Assignees
1 Participants
Notifications
Due Date
No due date set.
Dependencies

No dependencies set.

Reference: adam/Piwigo#1166