GBSA 9477/fix for demo#49
Conversation
…_infrastructure function
…A-9477/fix-for-demo
Added shellcheck disable comments to prevent warnings for variable expansion.
![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 adapts the Cybercafe backend for Termux, refactors iptables management with rule comments for improved cleanup, and configures lighttpd for PHP. Critical feedback identifies syntax errors in shell traps and a logic issue where iptables existence checks fail to account for new rule comments, causing duplicates. Additionally, converting test utilities from mocks to live commands is flagged as a risk, and hardcoded paths and IP addresses should be generalized to improve portability and reliability.
| { | ||
| #trap will catch any errors that occur and write the line number to the error.log file | ||
| trap 'echo -e "$(date '+%Y-%m-%dT%H:%M:%S%z') Error in Cybercafe_setupFunction.sh: Line ${LINENO}\n" >> error.log' ERR | ||
| trap 'echo -e "$(date '+%Y-%m-%dT%H:%M:%S%z') Error in Cybercafe_setupFunctions.sh: Line ${LINENO}\n" >> error.log' ERR |
There was a problem hiding this comment.
The trap command is syntactically incorrect because the single quotes around the date format string ('+%Y-%m-%dT%H:%M:%S%z') terminate the outer single-quoted string. This will cause a shell syntax error when the script is parsed. You should remove the inner single quotes or use a different quoting strategy to ensure the entire command is treated as a single string for the trap.
| trap 'echo -e "$(date '+%Y-%m-%dT%H:%M:%S%z') Error in Cybercafe_setupFunctions.sh: Line ${LINENO}\n" >> error.log' ERR | |
| trap 'echo -e "$(date +%Y-%m-%dT%H:%M:%S%z) Error in Cybercafe_setupFunctions.sh: Line ${LINENO}\n" >> error.log' ERR |
| { | ||
| # preserve existing trap / error logging | ||
| trap 'echo -e "$(date '+%Y-%m-%dT%H:%M:%S%z') Error in Cybercafe_setupFunction.sh: Line '${LINENO}' \n" >> error.log' ERR > /dev/null 2>> error.log | ||
| trap 'echo -e "$(date '+%Y-%m-%dT%H:%M:%S%z') Error in Cybercafe_setupFunctions.sh: Line '${LINENO}' \n" >> error.log' ERR > /dev/null 2>> error.log |
There was a problem hiding this comment.
Similar to line 96, this trap definition is broken by nested single quotes. Additionally, the use of '${LINENO}' (with single quotes inside the trap string) causes the line number to be evaluated at the time the trap is defined rather than when it is executed, which will log the wrong line number for errors. Removing the inner single quotes allows the variable to be preserved for execution-time evaluation.
| trap 'echo -e "$(date '+%Y-%m-%dT%H:%M:%S%z') Error in Cybercafe_setupFunctions.sh: Line '${LINENO}' \n" >> error.log' ERR > /dev/null 2>> error.log | |
| trap 'echo -e "$(date +%Y-%m-%dT%H:%M:%S%z) Error in Cybercafe_setupFunctions.sh: Line ${LINENO} \n" >> error.log' ERR > /dev/null 2>> error.log |
| iptables -t nat -I PREROUTING 1 -p tcp -i "${HS_INTERFACE}" \ | ||
| -m comment --comment "cybercafe-dnat" \ | ||
| -j DNAT --to-destination "${LOCAL_IP}:80" |
There was a problem hiding this comment.
This rule includes a comment (-m comment --comment "cybercafe-dnat"), but the check at line 135 (which determines whether to run this block) does not include it. Since iptables -C requires an exact match, it will fail to find the rule if it already exists with a comment, leading to duplicate rules being inserted every time setup_infrastructure is called. The same issue applies to the filter rules added in lines 154-160. You should update the check at line 135 to include the comment or ensure the check matches the inserted rule exactly.
|
|
||
| create_chain() { | ||
| echo "iptables -t mangle -N $2" | ||
| iptables -t mangle -N "$2" |
There was a problem hiding this comment.
Changing this helper from echo to actual iptables execution is a significant change for a test utility. If the test suite expects these helpers to mock behavior (as suggested by the file path and previous implementation), this change will cause tests to attempt real system modifications, which may fail due to lack of permissions or corrupt the host state during testing.
| iptables -t mangle -N "$2" | |
| echo "iptables -t mangle -N $2" |
| server.document-root = server_root + "/htdocs/" | ||
| index-file.names = ( "page.html", "page.php" ) | ||
| #server.document-root = server_root + "/htdocs/" | ||
| server.document-root = "/data/data/com.termux/files/home/Project_Cybercafe" |
There was a problem hiding this comment.
Hardcoding the absolute path to a specific user's home directory (/data/data/com.termux/files/home/Project_Cybercafe) makes the configuration non-portable and likely to break on other devices or for other users. Consider using the server_root variable or a relative path to improve maintainability.
server.document-root = server_root + "/Project_Cybercafe"
| var.allowedHostIPs = "10\.18\.120\.52|192\.168\.43\.79|127\.0\.0\.1" | ||
| var.hotspotNetwork = "192.168.43.0/24" |
There was a problem hiding this comment.
Hardcoding specific IP addresses like 192.168.43.79 and networks like 192.168.43.0/24 is fragile, as these often change in mobile hotspot environments. While the comment acknowledges this needs automation, using such specific values in a 'fix for demo' can still lead to immediate failure if the device's IP differs from the one hardcoded here.
Daniel-Lopez246
merged commit 34e88bf
into
Feature/Utility-Library-(ASU-CIDSE-2025-Fall)
1 participant
No description provided.