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

Issue #30 #33 Solved #35

Closed
wants to merge 4 commits into from

Conversation

anushkasaxena07
Copy link

@anushkasaxena07 anushkasaxena07 commented May 16, 2024

PR Description

The issue with the existing code is that it directly inserts user input into SQL queries, making it vulnerable to SQL injection attacks. To address this, the code needs to be refactored to use prepared statements with parameter binding. Prepared statements separate SQL logic from data, preventing malicious SQL code injection.

Solution:

Here's how to solve the issue:

  1. save_progress() function:

    • Refactor the code to use a prepared statement with parameter binding.
    • Bind parameters to the statement to ensure safe execution.
    • Execute the statement.
  2. delete_progress() function:

    • Refactor to use a prepared statement with parameter binding.
    • Bind the id parameter to the statement.
    • Execute the statement.
  3. save_restriction() function:

    • Loop through the $rid array to handle multiple rows.
    • Use prepared statements with parameter binding for both INSERT and UPDATE operations.
    • Execute the statements within the loop.
  4. save_evaluation() function:

    • Use a prepared statement with parameter binding to insert data into evaluation_list.
    • Loop through $qid to handle multiple question evaluations.
    • Use prepared statements with parameter binding to insert data into evaluation_answers.
    • Execute the statements within the loop.
  5. get_class() function:

    • Refactor to use a prepared statement with parameter binding.
    • Bind parameters for $fid and academic_id.
    • Execute the statement and fetch results.
  6. get_report() function:

    • Refactor to use a prepared statement with parameter binding.
    • Bind parameters for academic_id, faculty_id, subject_id, and class_id.
    • Execute the statement to get evaluation answers.
    • Use a prepared statement to get the total answered rows.
    • Calculate percentages and return the JSON-encoded data.

By implementing these changes, it ensures that user input is properly sanitized, and the application is protected from SQL injection attacks. Prepared statements with parameter binding handle user data safely, preventing malicious SQL injection attempts.

Related Issues: Issue for which you are raising a PR for Avoiding SQL injections using prepared statements of MySQLi #30
Closes #30

Issue

#30

Issue

#33

PR Description

  1. I modified the PHP script to include a condition for handling POST requests sent by the form.
  2. Upon receiving the form data, I perform checks to ensure the email doesn't already exist for another user using a SELECT query.
  3. If the email does exist, I echo 2 to indicate this condition.
  4. If the update is successful, I echo 1.
  5. If there's an error during the update process, I echo 0.
    This implementation ensures that the backend properly handles the scenario where the username already exists when updating user details.

Checklist

[ x] I have gone through the contributing guide
[ x] I have updated my branch and synced it with project main branch before making this PR
[x ] Is this a bug fix/enhancement/documentation changes
[ x] Part of GSSOC
[ x] Tested for any breaking changes
[x ] Other relevant checks completed

Undertaking

I declare that:

The content I am submitting is original and has not been plagiarized.
No portion of the work has been copied from any other source without proper attribution.
The work has been checked for plagiarism, and I assure its authenticity.
I understand that any violation of this undertaking may have legal consequences that I will bear and could result in the withdrawal of any recognition associated with the work.

[ x] I Agree

Copy link

@github-actions github-actions bot left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Welcome to Our repository.🎊 Thank you so much for taking the time to point this out.

@anushkasaxena07 anushkasaxena07 marked this pull request as ready for review May 16, 2024 15:05
@anushkasaxena07 anushkasaxena07 changed the title Issue #30 Solved Issue #30 #33 Solved May 17, 2024
manage_user.php Show resolved Hide resolved
@aswinikalyan30
Copy link
Collaborator

Needs more work - Please let me know if you have any trouble setting it up on your local and we can chat over discord

@anushkasaxena07
Copy link
Author

@aswinikalyan30 sorry for my negligence please have a look now
is it fine?

// Prepare the update query
$stmt = $this->db->prepare("UPDATE system_settings SET $data WHERE id = ?");
$stmt->bind_param($data_types."i", ...$data_values, $chk->fetch_array()['id']);
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Fatal error: Cannot use positional argument after argument unpacking in /Applications/MAMP/htdocs/Faculty_Evaluation_System-main/admin_class.php on line 354
Getting this error

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Ran it on my local and got this error - How are you testing your changes?

}
}
<?php
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Causing syntax issues on this line - remove it

$('#manage-user').submit(function(e){
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This function calls an empty url - but I see the query is written on top - how are you testing any of your changes?
Going forward I need screenshots added to the PR to see what output you are seeing on screen

@aswinikalyan30
Copy link
Collaborator

Hey @anushkasaxena07 - any progress?

@anushkasaxena07
Copy link
Author

Still working on it

@aswinikalyan30
Copy link
Collaborator

Hey @anushkasaxena07 this has been open for too long, if you're not able to solve it will assign to someone else

@aswinikalyan30
Copy link
Collaborator

Closing this PR as no working changes are made for more than 2 weeks and is blocking contributions from others

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Feat: Avoiding SQL injections using prepared statements of MySQLi (Open for contribution)
2 participants