Gbsa 9477/fix for demo#48
Conversation
![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 the Termux environment by updating shebangs, hardcoding specific paths, and refactoring iptables rule management to use comments for easier cleanup. Feedback focuses on several technical risks: the fragile string substitution logic used to delete iptables rules, the lack of portability due to hardcoded Termux-specific paths and user home directories, and the potential for unintended process termination when using pkill with regex patterns. Additionally, a change to a test helper now executes system commands directly, which may cause undesirable side effects during testing.
| iptables -t filter -S FORWARD 2>> error.log | grep 'cybercafe-block-' | while read -r rule; do | ||
| del_rule="${rule/-A/-D}" | ||
| iptables -t filter $del_rule > /dev/null 2>> error.log || true | ||
| done |
There was a problem hiding this comment.
The logic for deleting rules by string substitution is fragile. If a rule contains spaces within an argument (e.g., in a comment), the unquoted expansion of $del_rule will cause the iptables command to fail. This applies to the NAT PREROUTING cleanup loop as well (lines 292-295). Consider using a more robust way to parse and execute the deletion, or explicitly delete the known rules added during setup.
|
|
||
| ##VARIABLES## | ||
| BASE_PATH="$(cd "$(dirname "${BASH_SOURCE[0]}")" && pwd)" | ||
| UTIL_PATH="/data/data/com.termux/files/usr/bin" |
There was a problem hiding this comment.
Hardcoding UTIL_PATH to a Termux-specific directory (/data/data/com.termux/files/usr/bin) reduces the portability of the script. It is better to rely on the system PATH or implement a check to detect the environment and set paths accordingly.
| function command_kill | ||
| { | ||
| killall -9 Cybercafe_daemon.sh | ||
| pkill -f "$BASE_PATH/Cybercafe_daemon.sh" |
There was a problem hiding this comment.
pkill -f treats the pattern as a regular expression. If $BASE_PATH contains characters that are special in regex (such as .), it might match unintended processes. Additionally, it matches against the full command line, which could include other processes (like an editor) referencing the same path. Using a PID file is generally more reliable for process management.
| 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 server.document-root to a specific user's home directory makes the configuration non-portable and difficult to maintain across different installations. Consider using a path relative to the configuration file or an environment variable if supported by your deployment process.
|
|
||
| create_chain() { | ||
| echo "iptables -t mangle -N $2" | ||
| iptables -t mangle -N "$2" |
There was a problem hiding this comment.
This change converts a helper function that previously only printed a command into one that executes it. If this file is used in a testing context, it will now modify the host system's network configuration, which is usually undesirable for unit tests. Ensure that all callers of this helper expect this side effect.
…A-9477/fix-for-demo
1 participant
No description provided.