-
-
Notifications
You must be signed in to change notification settings - Fork 2.1k
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.
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
Blind sql injection fixes rework (#3284) #3316
base: master
Are you sure you want to change the base?
Conversation
80226a2
to
cfe9271
Compare
Applying this to Items the only column that sorts is the Item_id column. All other columns just sort the Item_id. |
Thanks for testing @odiea I have checked your comment. Which column were you trying to sort? If I sort on name or category it does seem to work sorting on name |
Ok weird, both cases seem to work fine on my end. I did force push a couple of times. Do you have the latest hash in use for the branch? |
I downloaded this branch. © 2010 - 2021 · opensourcepos.org · 3.3.5 - 4b12b7. |
Ok thanks for the feedback, I have deployed the version to dev now and was checking, the name field does not sort correctly. I think the others do in fact. |
Here is the code that I am testing out in Items. |
Think it should be cfe927 |
Indeed there is still issue, I'll try to debug it, thanks for the feedback |
I tried changing this and it appears to sort the columns correctly now. |
cfe9271
to
37942eb
Compare
Did another push and seems better now on dev. |
The only quirk I see now is on an older established db quantities does not sort? Looks ok on the Demo |
@odiea you mean the regular quantities field? |
Yes the Quantity field in the Items table |
37942eb
to
e9bf68d
Compare
@odiea should be fixed now. |
Yes it is. Works good now. |
What is the issue with placing the id before the $field? I have had to do this to get some of the other tables to sort correctly. I guess the way I see it is the first table sort is the id and if that is not chosen then the fields are chosen. |
Well what the logic does it it checks if the supplied sorting column exists in the db, then applies it. If some hacker tries to pass in some SQL for injection, it will be ignored as we only allow to sort on db column names. |
But this does not appear to work correctly. I tried sorting in Customers, Suppliers and Employees and columns do not sort properly. The columns appear to sort to the id and not the column. |
Indeed well spotted, I think those three models need one more adapation |
e9bf68d
to
1061079
Compare
Try something on Gift cards and you may be done with this |
1061079
to
9db27b2
Compare
Ok the columns for giftcards are extended now. I've gotten two more security bug reports through this huntr.dev platform today. Not sure how this works but it seems that some researchers are looking for security bugs in ospos now.. which is good. |
9db27b2
to
aeeac30
Compare
The sale table might not work correctly yet.. The current approach will do some extra queries to fetch the database column names, which do not necessarily map to the column headers names. I had a further look and there might be a better solution. The header names are already defined in the tabular_helper, but they are not in an accessible place. It might be better to extract those header arrays somewhere and then check for the field presence in each array separately. |
I should revisit this fix at some point.. |
Links to #3965 |
@jekkos can we merge this into ci4-branch instead of master? This fix will likely need to be applied to all sort and order fields in the various searches to close the vulnerability. I realize it will likely mean porting the changes over to a new branch based on ci4-branch, but I'd hate to duplicate work. |
No correct we need to target ci4-branch here. I'll try to rebase perhaps somewhere next week, quite busy now as I'm leaving on holidays also monday. |
I have reinstated the fix from a month ago. Want to get this out to release the 3.3.6.