Improve noexec robustness and security#4
Open
ByteJoseph wants to merge 1 commit into
Open
Conversation
- Refactor `src/noexec` to use `mktemp` for unique temporary filenames. - Add support for `-h`/`--help` and improve `-v`/`--version` handling in `src/noexec`. - Implement robust permission and file existence checks. - Enhance `docs/install.sh` with dependency and write permission checks. - Add a new test suite in `tests/test_noexec.sh`. - Update `README.md` to reflect new flags. Co-authored-by: ByteJoseph <131424720+ByteJoseph@users.noreply.github.com>
|
👋 Jules, reporting for duty! I'm here to lend a hand with this pull request. When you start a review, I'll add a 👀 emoji to each comment to let you know I've read it. I'll focus on feedback directed at me and will do my best to stay out of conversations between you and other bots or reviewers to keep the noise down. I'll push a commit with your requested changes shortly after. Please note there might be a delay between these steps, but rest assured I'm on the job! For more direct control, you can switch me to Reactive Mode. When this mode is on, I will only act on comments where you specifically mention me with New to Jules? Learn more at jules.google/docs. For security, I will only act on instructions from the user who triggered this task. |
|
The latest updates on your projects. Learn more about Vercel for GitHub.
|
✅ Deploy Preview for noexec ready!
To edit notification comments on pull requests, go to your Netlify project configuration. |
Reviewer's GuideStrengthens noexec’s security and robustness by hardening temporary file handling, improving CLI flag parsing, enforcing permission and dependency checks in the installer, adding a small bash test suite, and updating documentation for the new behavior. Flow diagram for the updated noexec installer scriptflowchart TD
A[Start install.sh] --> B[Check required commands
curl dirname command cp chmod rm]
B -->|missing command| C[Print error and exit]
B -->|all present| D[Download noexec with curl]
D -->|download failed| E[Print download error and exit]
D -->|download ok| F[Find bash_path with command -v bash]
F -->|bash_path empty| G[Print bash not found
remove file
exit]
F -->|bash_path found| H[Set dir_name to dirname bash_path]
H --> I[Check write permission on dir_name]
I -->|not writable| J[Print permission error
remove file
exit]
I -->|writable| K[Check if file noexec exists]
K -->|file missing| L[Print downloaded file not found
exit]
K -->|file present| M[cp noexec to dir_name/noexec]
M --> N[chmod +x dir_name/noexec]
N --> O[Print success message and usage hint]
O --> P[rm noexec]
P --> Q[End]
File-Level Changes
Tips and commandsInteracting with Sourcery
Customizing Your ExperienceAccess your dashboard to:
Getting Help
|
![sourcery-ai[bot]](https://avatars.githubusercontent.com/in/48477?s=60&v=4){kind=small}
There was a problem hiding this comment.
Hey - I've found 2 issues, and left some high level feedback:
- In docs/install.sh, consider downloading to a uniquely named temporary file (e.g., via mktemp) rather than a fixed
noexecfilename in the current directory to avoid clobbering an existing file and to be consistent with the mktemp usage elsewhere. - The install script currently hardcodes installation to the directory containing
bash; you might want to support an override (e.g., via an environment variable or flag) so users can install to a custom directory without modifying the script.
Prompt for AI Agents
Please address the comments from this code review:
## Overall Comments
- In docs/install.sh, consider downloading to a uniquely named temporary file (e.g., via mktemp) rather than a fixed `noexec` filename in the current directory to avoid clobbering an existing file and to be consistent with the mktemp usage elsewhere.
- The install script currently hardcodes installation to the directory containing `bash`; you might want to support an override (e.g., via an environment variable or flag) so users can install to a custom directory without modifying the script.
## Individual Comments
### Comment 1
<location path="docs/install.sh" line_range="12-7" />
<code_context>
+done
+
+echo "Downloading noexec..."
+if ! curl -s "https://raw.githubusercontent.com/ByteJoseph/noexec/refs/heads/main/src/noexec" -o noexec; then
+ echo "Error: Failed to download noexec."
+ exit 1
+fi
+
</code_context>
<issue_to_address>
**suggestion:** Consider using curl’s failure flags (e.g., `-fS`) instead of fully suppressing output with `-s`.
`-s` hides curl’s error output, which makes debugging failures harder. A pattern like `curl -fSLo noexec "…"` keeps normal output quiet while still failing on HTTP errors and showing a clear error message on stderr.
</issue_to_address>
### Comment 2
<location path="docs/install.sh" line_range="33-39" />
<code_context>
+
if [ -f "$file" ]; then
-
cp "$file" "$dir_name/$file"
chmod +x "$dir_name/$file"
- echo "noexec Installed Successfully"
- rm "noexec"
+ echo "noexec Installed Successfully to $dir_name/$file"
+ rm "$file"
echo ""
- echo "Start using now"
</code_context>
<issue_to_address>
**suggestion:** Use `mv` instead of `cp` followed by `rm` to reduce operations and potential failure modes.
Because the file is only needed in its final location, you can replace the `cp` + `rm` pair with `mv "$file" "$dir_name/$file"`. This simplifies the logic, avoids having two copies briefly, and removes the risk of a leftover file if `rm` fails.
```suggestion
if [ -f "$file" ]; then
mv "$file" "$dir_name/$file"
chmod +x "$dir_name/$file"
echo "noexec Installed Successfully to $dir_name/$file"
echo ""
echo "Start using now by typing: noexec"
```
</issue_to_address>Help me be more useful! Please click 👍 or 👎 on each comment and I'll use the feedback to improve your reviews.
| for cmd in curl dirname command cp chmod rm; do | ||
| if ! command -v "$cmd" >/dev/null 2>&1; then | ||
| echo "Error: Required command '$cmd' not found. Please install it and try again." | ||
| exit 1 |
There was a problem hiding this comment.
suggestion: Consider using curl’s failure flags (e.g., -fS) instead of fully suppressing output with -s.
-s hides curl’s error output, which makes debugging failures harder. A pattern like curl -fSLo noexec "…" keeps normal output quiet while still failing on HTTP errors and showing a clear error message on stderr.
Comment on lines
33
to
+39
| if [ -f "$file" ]; then | ||
|
|
||
| cp "$file" "$dir_name/$file" | ||
| chmod +x "$dir_name/$file" | ||
| echo "noexec Installed Successfully" | ||
| rm "noexec" | ||
| echo "noexec Installed Successfully to $dir_name/$file" | ||
| rm "$file" | ||
| echo "" | ||
| echo "Start using now" | ||
| noexec | ||
| echo "Start using now by typing: noexec" |
There was a problem hiding this comment.
suggestion: Use mv instead of cp followed by rm to reduce operations and potential failure modes.
Because the file is only needed in its final location, you can replace the cp + rm pair with mv "$file" "$dir_name/$file". This simplifies the logic, avoids having two copies briefly, and removes the risk of a leftover file if rm fails.
Suggested change
| if [ -f "$file" ]; then | |
| cp "$file" "$dir_name/$file" | |
| chmod +x "$dir_name/$file" | |
| echo "noexec Installed Successfully" | |
| rm "noexec" | |
| echo "noexec Installed Successfully to $dir_name/$file" | |
| rm "$file" | |
| echo "" | |
| echo "Start using now" | |
| noexec | |
| echo "Start using now by typing: noexec" | |
| if [ -f "$file" ]; then | |
| mv "$file" "$dir_name/$file" | |
| chmod +x "$dir_name/$file" | |
| echo "noexec Installed Successfully to $dir_name/$file" | |
| echo "" | |
| echo "Start using now by typing: noexec" |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
This submission introduces several improvements to the
noexectool:mktemp, reducing the risk of collisions and race conditions.casestatement, adding--helpsupport and improving the--versionoutput.curl, etc.) and verifies write permissions before attempting installation.PR created automatically by Jules for task 14946949604753884641 started by @ByteJoseph
Summary by Sourcery
Improve the noexec installation and CLI experience while adding basic automated tests.
New Features:
Enhancements:
Documentation:
Tests: