My-Little-Forum / mylittleforum

A simple PHP and MySQL based internet forum that displays the messages in classical threaded view (tree structure)
GNU General Public License v3.0
121 stars 47 forks source link

Signature length #655

Open loesler opened 1 year ago

loesler commented 1 year ago

The length of the signature is restricted by the forum setting $settings['signature_maxlength']. However, there is a further (not visible) restriction in the SQL column definition, i.e., signature varchar(255) NOT NULL default. On the one hand, both restrictions are redundantly and on the other hand, the $settings['signature_maxlength'] becomes meaningless if the value is greater than 255. Do we really need $settings['signature_maxlength']?

auge8472 commented 1 year ago

Do we really need $settings['signature_maxlength']?

This question is valid in many cases. Do we need a doubled restriction for user names, subjects and a few more settings, along with the additional limits on word length? There are hardcoded limits for the associated database columns. IMHO we should make use of them. On Stack Overflow I found the following code snippets for getting the possible length of a column.

Reply (and accepted answer) of P James, 2016-10-16:

SELECT CHARACTER_MAXIMUM_LENGTH FROM information_schema.COLUMNS WHERE TABLE_SCHEMA = 'Database' AND TABLE_NAME = 'Table' AND COLUMN_NAME = 'Field'

Reply of Vignesh Kumar A, 2014-02-14:

SELECT column_name, 
       character_maximum_length 
FROM   information_schema.columns 
WHERE  table_schema = 'websi_db1' 
       AND table_name = 'thread' 
       AND column_name = 'title'

This is code to determine the column length at script runtime. This might have relevant performance issues for relevant scripts but it is IMHO the only clean possible solution. The alternative would be to read it once from the installation script or the database and to store it somewhere. The first place, that comes into my mind would be the settings table but with this solution we would end at the starting point (a setting that can differ from and exceed the techical limits).

In individual cases the column length might be supplemented by a word length limit (i.e. possibly for the subject). But that's a separate story (IMHO).

loesler commented 1 year ago

Storing the length in a further column is like the current solution i.e. redundantly. In my opinion, it is not a reliable solution.

Since we're talking about admin options, I can't really figure out a performance issue - please note that we're talking about the maximum value, which can only be set by the forum admin. This value is validated once the admin changed the forum settings but is not used to validate user input.

auge8472 commented 1 year ago

Removing the settings and reading the real maximum sizes from the database definitions instead would make it necessary to do this every time, when affected values should be checked. So this would be not an issue for admins only.

This value is validated once the admin changed the forum settings but is not used to validate user input.

When the settings to check the lenght of usernames, e-mail-addresses, subjects, etc. pp are not in use for checking the matching input, for what they are there?

loesler commented 1 year ago

Up to now, the admin is able to restrict the length of some input fields e.g. the signature, the user name, etc. All these inputs are checked against the limits defined by the admin, which are not identical to the defined field length in the database. Thus, the value set by the admin must be validated w.r.t. to the database definition. At the moment, the admin can set the signature length to say 1000 characters without getting an error. This value is invalid w.r.t. to the definition of the database field. The user input is validated w.r.t. the admin settings and not w.r.t. the database definition.

auge8472 commented 1 year ago

It seems like we are talking past each other.

Up to now, the admin is able to restrict the length of some input fields …

Yes and that's the issue here.

One solution would be to check the settings, set by the admin against the database definition to ensure the validity of the setting. The input would be validated against the settings. It seems to me, that this is the way you want to go. Is that correct?

Another solution would be to dispence the settings (where possible) and to perform the input checks directly against the values from the database definitions. The price would be, that those database definitions have to be read from the database in every single script run when input of anyone should be validated. That was my proposal.

loesler commented 1 year ago

It seems to me, that this is the way you want to go. Is that correct?

This is my preferred solution, yes.

Another solution would be to dispence the settings

No, it isn't. For various reasons, the admin may want to limit the number of characters for some inputs. If we use the database definition, it would no longer be possible to restrict the input.

auge8472 commented 1 year ago

Ok. The task is to check the validity of lenght restriction settings against the column size restrictions from the database. Correct?

loesler commented 1 year ago

Yes, I think we need some procedures to validate the input of an admin. Maybe, the definition of the column size is not the best way but it is one way. We can also define the parameter range in further columns for e.g. boolean etc.

auge8472 commented 1 year ago

I once made an attempt in MLF1 to make the settings clearer by grouping them, providing input types (at that time: text, checkbox, radio and select) and for a few cases allowed values or function names for collecting allowed values from the code or file system (in example for the available languages) (see the query for available informations). I would choose another solution nowadays, i.e. with a JSON object for the configuration details of a single settings entry.

I also made a first design sketch for implementing a modernised version for MLF2 (with the mentioned JSON configuration) but currently I have no access to the machine, where the branch is stored. Tomorrow (2023-01-04) I will have and then I can upload the branch to Github for inspection.

auge8472 commented 1 year ago

While overworking the configuration wiki page I came across a possible issue with another length limitation. There is a pair of settings named name_maxlenght and name_word_maxlenght as well as a setting named username_maxlength. I didn't take a look into the code but I assume the settings beginning with name_ are meant to restrict names of unregistered posters but the setting username_maxlength restricts the length of names of registered users. Wouldn't it be more consistent to set the restrictions in the same way for all possible cases?

That way we would have one ruleset for unregistered and also registered users. Additionally we could have a username word length restriction also for registered users, in case we would keep such a restriction (I have never been convinced by this restriction).

loesler commented 1 year ago

Yes, you are right. It should be identical.