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.

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

escape variables in question bank columns sql #277

Draft
wants to merge 3 commits into
base: main
Choose a base branch
from

Conversation

dionysius
Copy link
Member

@dionysius dionysius commented Oct 13, 2020

While all used variables are currently safe, this must be considered accidental. We should escape all usages of variables in this query. The question bank column classes really need an overhaul to correctly escape input data!

@dionysius dionysius marked this pull request as draft October 13, 2020 17:42
@dionysius dionysius changed the title fix #275, allow a user to see his own question, even when hidden escape variables in question bank columns sql Oct 14, 2020
@dionysius dionysius force-pushed the escape_variables_in_question_bank_columns branch from 540abab to 39f3856 Compare October 14, 2020 12:48
@dionysius dionysius marked this pull request as ready for review October 14, 2020 12:51
@dionysius
Copy link
Member Author

dionysius commented Oct 14, 2020

This PR should simplify the use of the "current" user and studentquiz id in the question bank column queries. First it improves maintainability by not needing to define those all the time, secondly in case additional sanitation has to be made, it is in a well defined place, and thirdly it reduces load to prepare the "big query" by not needing to find the additional field values.

Moodle has the option to use :variablename in queries, but unfortunately you cannot use that multiple times. This way, you have a joined "table" current providing those fields as static select elements and you can always use those as conditions for further joins. I expect the performance also to be better as the database can earlier filter the joined tables according to the on conditions. But that statement is just from expectation, there's a chance that I'm totally wrong and its worse and thus needs to be tested as well.

@dionysius
Copy link
Member Author

dionysius commented Oct 15, 2020

With help of 0a8d71c:

TEST_PERF: create 1 instances
TEST_PERF: initialization complete
TEST_PERF: displaying question bank took 0.21114706993103 s
TEST_PERF: create 10 instances
TEST_PERF: initialization complete
TEST_PERF: displaying question bank took 0.52533197402954 s
TEST_PERF: create 100 instances
TEST_PERF: initialization complete
TEST_PERF: displaying question bank took 0.23308610916138 s

But this approach doesn't like pgsql. Putting PR aside for a bit

@dionysius dionysius marked this pull request as draft October 15, 2020 10:23
@dionysius dionysius added the later Not pressing right now, usually nice to have issues label Oct 15, 2020
@timhunt
Copy link
Member

timhunt commented Oct 15, 2020

I don't see why this would not work in Postgres. Perhaps you need to add ON 1 = 1?

In studentquiz_bank_view.php, you should be able to do JOIN (SELECT ? as userid, ? as studentquizid) current and put those values in the $params array.

@dionysius
Copy link
Member Author

I don't see why this would not work in Postgres. Perhaps you need to add ON 1 = 1?

I can give that a try.

In studentquiz_bank_view.php, you should be able to do JOIN (SELECT ? as userid, ? as studentquizid) current and put those values in the $params array.

Yes definitely, now we can do that.

@timhunt timhunt force-pushed the main branch 3 times, most recently from cfaee3b to 8f4fe71 Compare July 17, 2024 20:31
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
later Not pressing right now, usually nice to have issues
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants