Fixed CI Pipeline#42
Conversation
…y and consistency across scripts
- Updated variable quoting for better safety and to prevent word splitting in Cybercafe_setupFunctions.sh, Cybercafe_testbed.sh, cybercafe.sh, and run.sh. - Enhanced error logging with clearer messages and consistent formatting. - Improved command execution in shutdown_infrastructure and setup_infrastructure functions to ensure proper error handling. - Added shellcheck directives to suppress specific warnings for better code clarity. - Adjusted user prompts to use read -r for safer input handling. - Cleaned up deprecated rules and comments in iptables configurations. - Ensured consistent use of double quotes around variables to prevent issues with spaces and special characters.
…s commands and test helpers
Summary of ChangesHello, I'm Gemini Code Assist1! I'm currently reviewing this pull request and will post my feedback shortly. In the meantime, here's a summary to help you and other reviewers quickly get up to speed! This pull request significantly enhances the robustness and maintainability of the Cybercafe backend shell scripts. The changes primarily focus on adopting modern shell scripting best practices, improving error handling, and ensuring consistent variable usage. These updates are crucial for stabilizing the CI pipeline and preventing common shell-related issues, leading to more reliable script execution and easier debugging. Highlights
🧠 New Feature in Public Preview: You can now enable Memory to help Gemini Code Assist learn from your team's feedback. This makes future code reviews more consistent and personalized to your project's style. Click here to enable Memory in your admin console. Ignored Files
Using Gemini Code AssistThe full guide for Gemini Code Assist can be found on our documentation page, here are some quick tips. Invoking Gemini You can request assistance from Gemini at any point by creating a comment using either
Customization To customize Gemini Code Assist for GitHub experience, repository maintainers can create a configuration file and/or provide a custom code review style guide (such as PEP-8 for Python) by creating and adding files to a Limitations & Feedback Gemini Code Assist may make mistakes. Please leave feedback on any instances where its feedback is incorrect or counter productive. You can react with 👍 and 👎 on @gemini-code-assist comments. If you're interested in giving your feedback about your experience with Gemini Code Assist for GitHub and other Google products, sign up here. Footnotes
|
Daniel-Lopez246
merged commit d1cbb32
into
Feature/Utility-Library-(ASU-CIDSE-2025-Fall)
![gemini-code-assist[bot]](https://avatars.githubusercontent.com/in/956858?s=60&v=4){kind=small}
There was a problem hiding this comment.
Code Review
This pull request primarily focuses on improving the robustness, security, and maintainability of the Cybercafe backend shell scripts. Key changes include updating shebangs to #!/usr/bin/env bash, consistently quoting variables and using modern arithmetic expansion, and refactoring error handling and command execution, notably changing eval "$@" to "$@" in run_cmd for security. Path handling in test scripts was also improved for portability. During the review, a critical bug was identified in remove_session where the global variable $I was incorrectly used instead of the function argument $1, potentially leading to incorrect session data archiving. The change from eval "$@" to "$@" in run_cmd introduced a critical breaking change, requiring all call sites to be updated to pass command arguments separately. Additionally, the date parsing and calculation logic in check_internet_sessions was noted as complex and brittle, incorrect quoting for the --to-destination argument in iptables commands was found, and an unsafe user input parsing method in cybercafe.sh was highlighted.
| { | ||
| RESPONSE=$(sqlite3 "${DATABASE_PATH}" "SELECT * FROM internet_sessions WHERE table_index=${I}") > /dev/null 2>> error.log | ||
| DATETIME=$(date '+%Y-%m-%d %H:%M:%S') > /dev/null | ||
| SESSION_TX=$(echo "$RESPONSE" | cut -f 5 -d '|') > /dev/null | ||
| SESSION_RX=$(echo "$RESPONSE" | cut -f 6 -d '|') > /dev/null | ||
| USER_ID=$(echo "$RESPONSE" | cut -f 2 -d '|') > /dev/null | ||
| USER_IP=$(echo "$RESPONSE" | cut -f 4 -d '|') > /dev/null | ||
| } 2>>error.log |
There was a problem hiding this comment.
There is a critical bug in this block. The function remove_session is supposed to operate on the session index passed as argument $1. However, the SELECT query on line 125 uses the global variable $I to fetch session data. This can lead to archiving data for the wrong session if $I and $1 are different, or if the function is ever called from a context where $I is not set to the intended value.
All occurrences of $I within this function should be replaced with $1 to ensure it operates on the correct session index.
| { | |
| RESPONSE=$(sqlite3 "${DATABASE_PATH}" "SELECT * FROM internet_sessions WHERE table_index=${I}") > /dev/null 2>> error.log | |
| DATETIME=$(date '+%Y-%m-%d %H:%M:%S') > /dev/null | |
| SESSION_TX=$(echo "$RESPONSE" | cut -f 5 -d '|') > /dev/null | |
| SESSION_RX=$(echo "$RESPONSE" | cut -f 6 -d '|') > /dev/null | |
| USER_ID=$(echo "$RESPONSE" | cut -f 2 -d '|') > /dev/null | |
| USER_IP=$(echo "$RESPONSE" | cut -f 4 -d '|') > /dev/null | |
| } 2>>error.log | |
| { | |
| RESPONSE=$(sqlite3 "${DATABASE_PATH}" "SELECT * FROM internet_sessions WHERE table_index=${1}") > /dev/null 2>> error.log | |
| DATETIME=$(date '+%Y-%m-%d %H:%M:%S') > /dev/null | |
| SESSION_TX=$(echo "$RESPONSE" | cut -f 5 -d '|') > /dev/null | |
| SESSION_RX=$(echo "$RESPONSE" | cut -f 6 -d '|') > /dev/null | |
| USER_ID=$(echo "$RESPONSE" | cut -f 2 -d '|') > /dev/null | |
| USER_IP=$(echo "$RESPONSE" | cut -f 4 -d '|') > /dev/null | |
| } 2>>error.log |
| else | ||
| # execute and append stderr to error.log so we keep original behavior | ||
| eval "$@" 2>> error.log || true | ||
| "$@" 2>> error.log || true |
There was a problem hiding this comment.
Replacing eval "$@" with "$@" is a great security improvement. However, this change breaks all existing calls to run_cmd, which pass the entire command as a single quoted string.
With "$@", the function will try to execute a command whose name is the entire string, including spaces and arguments, which will fail.
To fix this, you need to update all call sites of run_cmd to pass the command and arguments as separate words, not as a single string. For example:
run_cmd "iptables -t mangle -D FORWARD ..."
should become:
run_cmd iptables -t mangle -D FORWARD ...
Additionally, the run_cmd function now handles 2>> error.log || true, so these should be removed from the call sites.
| run_cmd "iptables -t mangle -D FORWARD -i '${HS_INTERFACE}' -j iptmon_tx > /dev/null 2>> error.log || true" | ||
| run_cmd "iptables -t mangle -D POSTROUTING -o '${HS_INTERFACE}' -j iptmon_rx > /dev/null 2>> error.log || true" |
There was a problem hiding this comment.
These calls to run_cmd are now broken due to the change from eval "$@" to "$@" in the function definition. The entire command is being passed as a single string argument.
The calls should be changed to pass arguments separately. Also, the redirections and || true are now handled by run_cmd and should be removed from the call.
| run_cmd "iptables -t mangle -D FORWARD -i '${HS_INTERFACE}' -j iptmon_tx > /dev/null 2>> error.log || true" | |
| run_cmd "iptables -t mangle -D POSTROUTING -o '${HS_INTERFACE}' -j iptmon_rx > /dev/null 2>> error.log || true" | |
| run_cmd iptables -t mangle -D FORWARD -i "${HS_INTERFACE}" -j iptmon_tx | |
| run_cmd iptables -t mangle -D POSTROUTING -o "${HS_INTERFACE}" -j iptmon_rx |
| SESSION_AGE=$((86400 * 10#$(echo "$RESPONSE" | awk '{print $1}' | cut -f 4 -d '-') + 3600 * 10#$(echo "$RESPONSE" | awk '{print $2}' | cut -f 1 -d ':') + 60 * 10#$(echo "$RESPONSE" | awk '{print $2}' | cut -f 2 -d ':') + 10#$(echo "$RESPONSE" | awk '{print $2}' | cut -f 3 -d ':' | cut -f 1 -d '.'))) | ||
| SESSION_IDLETIME=$((86400 * 10#$(echo "$RESPONSE2" | awk '{print $1}' | cut -f 4 -d '-') + 3600 * 10#$(echo "$RESPONSE2" | awk '{print $2}' | cut -f 1 -d ':') + 60 * 10#$(echo "$RESPONSE2" | awk '{print $2}' | cut -f 2 -d ':') + 10#$(echo "$RESPONSE2" | awk '{print $2}' | cut -f 3 -d ':' | cut -f 1 -d '.'))) |
There was a problem hiding this comment.
This date parsing and calculation logic is very complex and brittle. It relies on parsing the output of a non-standard timediff function with a long chain of echo, awk, and cut commands. This is hard to read, maintain, and prone to breaking if the timediff output format changes.
Consider performing the date calculations directly within SQLite using standard functions like julianday or strftime to get the difference in seconds. This would be more robust and efficient.
For example:
SELECT (julianday('now', 'localtime') - julianday(datetime_created)) * 86400 FROM internet_sessions WHERE table_index=${I}|
|
||
| #special rules that service the cybercafe system by blocking certain traffic over hotspot interface | ||
| iptables -t nat -C PREROUTING -p tcp -i ${HS_INTERFACE} -j DNAT --to-destination ${LOCAL_IP}:80 > /dev/null 2>> error.log #checks to see that rules don't exist | ||
| iptables -t nat -C PREROUTING -p tcp -i "${HS_INTERFACE}" -j DNAT --to-destination "${LOCAL_IP}":80 > /dev/null 2>> error.log #checks to see that rules don't exist |
There was a problem hiding this comment.
The quoting for the --to-destination argument appears incorrect. It is currently "${LOCAL_IP}":80, which will fail if $LOCAL_IP is empty. It should be "${LOCAL_IP}:80" to ensure the IP and port are treated as a single argument and to handle the case where $LOCAL_IP might be empty. A similar issue exists on line 132.
| iptables -t nat -C PREROUTING -p tcp -i "${HS_INTERFACE}" -j DNAT --to-destination "${LOCAL_IP}":80 > /dev/null 2>> error.log #checks to see that rules don't exist | |
| iptables -t nat -C PREROUTING -p tcp -i "${HS_INTERFACE}" -j DNAT --to-destination "${LOCAL_IP}:80" > /dev/null 2>> error.log #checks to see that rules don't exist |
| read -r user_input | ||
| args=() | ||
| for i in $user_input; do | ||
| args+=($i) | ||
| args+=("$i") | ||
| done |
There was a problem hiding this comment.
Parsing user input with for i in $user_input is unsafe. It performs word splitting on the unquoted variable, but it also subjects the input to glob expansion. If a user enters *, it will be expanded to all files in the current directory.
A safer and simpler way to read user input into an array is to use read -a.
| read -r user_input | |
| args=() | |
| for i in $user_input; do | |
| args+=($i) | |
| args+=("$i") | |
| done | |
| read -r -a args |
1 participant
No description provided.