Merge Feature/utility library (asu cidse 2025 fall) into main#53
Conversation
removed echo lines since daemon will now mostly be called from command line also uncommented clear_internet_sessions so that shutdown will clear internet sessions
had error with session_age and resolving some of the variables as octal which would give a number outside of base error when '09' or '08' appeared changed some other stuff as well
for iptmon_rx to work properly it needs to intercept the packet before it leaves the wlan1 interface which doesn't happen from the FORWARD mangle table also some misc changes
not in use anymore and code is completely irrelevent
general debugging and making webpage easier to navigate
lots of changes
new simple generic logo added as well
after getting a better understanding of primary keys, I updated primary keys to just be user_id for the first 2 tables. Certain columns will still need to be unique such at table_index and others. But this will have to be done externally.
added "list rules" to list out iptable rules
added "globalfunctions.php" to increase reusability, still looking to create a global CSS file for common html formatting
Fixed CI Pipeline
…s commands for correct syntax
Just some bug fixes and an additional suppress
…cution wrapping full cybercafe lifecycle: preflight → build → run → status → shutdown and imdempotent behavior by skipping builds if it already exists and runs guards against duplicate starts from daemon. - Stage isolation: each stage callable independently or as full 'all' run - Logs all output to orchestrator.log for validation evidence - Targets T95 device via Termux with root access required - Fixes --rebuild flag typo in build stage warn message
…nexpectedly so replace with the proper if block.
… variable with globbing and word splitting warnings.
…read the file directly.
…o keep consistent over shellcheck style preference and readability.
…cution Stage isolation: each stage callable independently or as full 'all' run Logs all output to orchestrator.log for validation evidence Targets T95 device via Termux with root access required Fixes --rebuild flag typo in build stage warn message
…_infrastructure function
…A-9477/fix-for-demo
Added shellcheck disable comments to prevent warnings for variable expansion.
Backend is now stable for demo
The log checks run regardless of whether the daemon shows Running or Stopped so you always capture the database state even if the ps compatibility bug is still causing status check to fail.
…oss-platform compatibility
There was a problem hiding this comment.
Code Review
This pull request introduces the Cybercafe system, a captive portal and hotspot management solution comprising a backend daemon, infrastructure setup scripts, and a PHP-based web interface. The code review identifies several critical security vulnerabilities, specifically SQL injection risks across multiple PHP files where user input is directly concatenated into queries. Additionally, the feedback provides actionable improvements for the shell scripts, such as handling null results from database queries to prevent arithmetic errors, avoiding redundant trap registrations in loops, and replacing hardcoded interface names with environment variables for better configurability.
| } | ||
| $password=hashPassword($password); | ||
| #check that username is unique | ||
| $response = $db->query("SELECT 1 FROM users WHERE username='".$username."'"); |
There was a problem hiding this comment.
The username is directly concatenated into the SQL query, making the application vulnerable to SQL injection. Use prepared statements to safely handle user input.
$stmt = $db->prepare("SELECT 1 FROM users WHERE username=:username");
$stmt->bindValue(':username', $username, SQLITE3_TEXT);
$response = $stmt->execute();| { | ||
| $password = hashPassword($password); | ||
| $db = global_createDatabaseObj(); | ||
| $response = $db->query("SELECT * FROM users WHERE username='".$username."'"); |
There was a problem hiding this comment.
Directly embedding the $username variable into the SQL query is a security risk (SQL injection). Always use prepared statements for queries involving user-provided data.
$stmt = $db->prepare("SELECT * FROM users WHERE username=:username");
$stmt->bindValue(':username', $username, SQLITE3_TEXT);
$response = $stmt->execute();| if(isset($cookies['session_id'])) | ||
| { | ||
| $db = global_createDatabaseObj(); | ||
| $response = $db->query("SELECT user_id FROM internet_sessions WHERE session_id='".$cookies['session_id']."'"); |
There was a problem hiding this comment.
The session ID from cookies is used directly in a database query without sanitization, leading to a potential SQL injection vulnerability.
$stmt = $db->prepare("SELECT user_id FROM internet_sessions WHERE session_id=:session_id");
$stmt->bindValue(':session_id', $cookies['session_id'], SQLITE3_TEXT);
$response = $stmt->execute();| { | ||
| trap 'echo -e "$(date) Error in Cybercafe_internetSessionFunctions.sh: Line ${LINENO}\n" >> error.log' ERR > /dev/null 2>> error.log | ||
| I=0 #iterator value | ||
| INDEX_LIMIT=$(($(sqlite3 "${DATABASE_PATH}" "SELECT MAX(table_index) FROM internet_sessions;")+1)) > /dev/null 2>> error.log #the maximum number of internet session entries |
There was a problem hiding this comment.
This arithmetic expansion will fail if the internet_sessions table is empty, as MAX(table_index) will return an empty string (NULL). Additionally, the redirection > /dev/null is applied to the variable assignment itself, which is unconventional. Consider using IFNULL in the SQL query and splitting the assignment from the arithmetic.
| INDEX_LIMIT=$(($(sqlite3 "${DATABASE_PATH}" "SELECT MAX(table_index) FROM internet_sessions;")+1)) > /dev/null 2>> error.log #the maximum number of internet session entries | |
| INDEX_LIMIT=$(sqlite3 "${DATABASE_PATH}" "SELECT IFNULL(MAX(table_index), 0) FROM internet_sessions;") 2>> error.log #the maximum number of internet session entries | |
| INDEX_LIMIT=$(( INDEX_LIMIT + 1 )) |
| SESSION_TX=$(($(iptables -t mangle -L iptmon_tx -vxn | grep "${USER_IP}" | awk '{print $2}'))) | ||
| SESSION_RX=$(($(iptables -t mangle -L iptmon_rx -vxn | grep "${USER_IP}" | awk '{print $2}'))) |
There was a problem hiding this comment.
Using arithmetic expansion $(()) directly on the output of a command pipeline is risky. If grep finds no matches, the resulting expression $(()) evaluates to 0 in Bash, but if it returns multiple values or unexpected characters, it will cause a syntax error. It's safer to capture the output into a variable first and provide a default value.
| SESSION_TX=$(($(iptables -t mangle -L iptmon_tx -vxn | grep "${USER_IP}" | awk '{print $2}'))) | |
| SESSION_RX=$(($(iptables -t mangle -L iptmon_rx -vxn | grep "${USER_IP}" | awk '{print $2}'))) | |
| SESSION_TX=$(iptables -t mangle -L iptmon_tx -vxn | grep "${USER_IP}" | awk '{print $2}') | |
| SESSION_TX=${SESSION_TX:-0} | |
| SESSION_RX=$(iptables -t mangle -L iptmon_rx -vxn | grep "${USER_IP}" | awk '{print $2}') | |
| SESSION_RX=${SESSION_RX:-0} |
|
|
||
| ### START OF PROGRAM EXECUTION ### | ||
| while true; do | ||
| trap 'echo -e "$(date) Error in Cybercafe_daemon.sh: Line ${LINENO}\n" >> error.log' ERR > /dev/null 2>> error.log |
| HS_STATUS_PREV="$HS_STATUS" | ||
|
|
||
| # From Chris's with better probing method to grab a specific IP pattern for the hotspot | ||
| ip add show dev wlan0 | grep 192\.168\.43\. > /dev/null 2>> error.log # Does it appear the hotspot is active? |
There was a problem hiding this comment.
The interface name wlan0 is hardcoded here, but the script elsewhere uses the variable ${HS_INTERFACE}. This could lead to incorrect status checks if the hotspot is configured on a different interface.
| ip add show dev wlan0 | grep 192\.168\.43\. > /dev/null 2>> error.log # Does it appear the hotspot is active? | |
| ip add show dev "${HS_INTERFACE}" | grep 192\.168\.43\. > /dev/null 2>> error.log # Does it appear the hotspot is active? |
| args=() | ||
| for i in $user_input; do | ||
| args+=("$i") | ||
| done |
There was a problem hiding this comment.
Parsing user input using a for loop over an unquoted variable is fragile as it relies on word splitting and does not handle quoted arguments correctly. Using read -a is a more robust way to populate an array from a string.
| args=() | |
| for i in $user_input; do | |
| args+=("$i") | |
| done | |
| read -r -a args <<< "$user_input" |
No description provided.