Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

security fixes #973

Merged
merged 6 commits into from
Aug 12, 2023
Merged

security fixes #973

merged 6 commits into from
Aug 12, 2023

Conversation

nielsdrost7
Copy link
Contributor

Description

Fix some security vulnerabilities

Related Issue

Fixed a number of security vulnerabilities:

FileName FieldName Fix type
X Mdl_email_templates.php from_name trim/xss_clean
X Mdl_clients.php client_language trim/xss_clean
X Mdl_clients.php client_country trim/xss_clean
X Mdl_custom_fields.php is_natural xss_clean
X Mailer/Controllers/Mailer.php to_email default xss_clean
X Quotes/Controllers/Ajax.php quote_id default xss_clean
X Sessions/Controllers/Sessions.php token check for alpha numeric characters
X Sessions/Controllers/Sessions.php email check for proper email address

Pull Request Checklist

  • My code follows the code formatting guidelines.
  • I have an issue ID for this pull request.
  • I selected the corresponding branch.
  • I have rebased my changes on top of the corresponding branch.

Issue Type

  • Bugfix

@nielsdrost7
trim/xss_clean posted from_name for the email_template. There was a p…
91fe83f 
…ossible vulnerability for that field
@nielsdrost7
Run posted quote_id through default xss_clean. There was a possible v…
987411d 
…ulnerability for that field
@nielsdrost7
Run posted client_country and client_language through xss_clean. Ther…
aa9a483 
…e was a possible vulnerability for those field
@nielsdrost7
Run posted is_natural through xss_clean. There was a possible vulnera…
1463fc6 
…bility for that field
@nielsdrost7
Run posted to_mail through xss_clean. There was a possible vulnerabil…
fd87ccc 
…ity for that field. Clean up some spacing issues
@nielsdrost7
nasty ones:
58a13a4 
- Properly check for the $token from the request, to see if they only have alpha_numeric values.
- Properly check email address if it's a valid email address
There were security vulnerabilities where this wasn't checked properly.
@nielsdrost7 nielsdrost7 marked this pull request as draft August 12, 2023 08:10
@nielsdrost7 nielsdrost7 marked this pull request as ready for review August 12, 2023 10:19
@nielsdrost7 nielsdrost7 merged commit aaa51ec into InvoicePlane:development Aug 12, 2023
1 check passed
@Verony-makesIT
Copy link
Contributor

@nielsdrost7
While testing the current IP development branch, I received the following error messages:
Unable to access an error message corresponding to your field name ...

  • client_country.(xss_clean) ->in client/form
  • Language.(xss_clean) -> in client/form
  • From Name.(xss_clean) ->in email_templates/form
    Screenshot003
    I can solve this myself (temporarily) by deactivating the lines e.g. 'rules' => 'trim|xss_clean' in their respective models files but I suspect this is not the intention...
    Can you take a look at this, please?
    Thnx.

@nielsdrost7
Copy link
Contributor Author

@Verony-makesIT can you make a PR where you remove the xss_clean from the rules from those fields?
those fields are integers and you can't do xss_clean on integers

@Verony-makesIT
Copy link
Contributor

@nielsdrost7, sorry but I really don't understand what you expect from me with your request to create a PR.
I just wanted to indicate what I have done to get your security fix (at the moment and for me) back working in my (local) development branch.
I also have no idea what this xss_clean functionality does, how it works at all, why xss_clean is used and how this problem can be solved.
I also do not understand why you indicate in your answer/question that these fields are integers while I see in the (ip_clients and _ip_email_templates) tables that these are "text and/or varchar" fields.
image
So a lot of questions I don't know the answer to.

Modifying code and then creating a PR of something I don't understand anything about, I'd rather leave that to "real developers" with more understanding than me...

nielsdrost7 added a commit that referenced this pull request Sep 17, 2023
@nielsdrost7
#973: Bug fixes after a report in the comments of PR #973: too many x…
8a1931c 
…ss_clean functions
@nielsdrost7 nielsdrost7 mentioned this pull request Sep 17, 2023
Merged
nielsdrost7 added a commit that referenced this pull request Sep 17, 2023
@nielsdrost7
#973: Applied xss_clean on too many places. This is the correction
07284d0
nielsdrost7 added a commit that referenced this pull request Sep 17, 2023
@nielsdrost7
#973: Quick commit
762bf12
nielsdrost7 added a commit that referenced this pull request Sep 17, 2023
@nielsdrost7
#973: Bug fixes after a report in the comments of PR #973 (#981)
1a625af 
* #973: Bug fixes after a report in the comments of PR #973: too many xss_clean functions

* #973: Applied xss_clean on too many places. This is the correction

* #973: Quick commit
nielsdrost7 added a commit that referenced this pull request Dec 16, 2023
@nielsdrost7
#973: Bug fixes after a report in the comments of PR #973 (#981)
1665d1d 
* #973: Bug fixes after a report in the comments of PR #973: too many xss_clean functions

* #973: Applied xss_clean on too many places. This is the correction

* #973: Quick commit
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers
No reviews
Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

None yet
2 participants
@nielsdrost7 @Verony-makesIT