Skip to content

🛡️ Sentinel: [MEDIUM] Fix insecure file permissions in backup script #38

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Open
kidchenko wants to merge 1 commit into
base: main
Choose a base branch
from
Open

🛡️ Sentinel: [MEDIUM] Fix insecure file permissions in backup script #38

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
4 changes: 4 additions & 0 deletions .jules/sentinel.md
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,4 @@
## 2026-02-22 - Insecure Default Permissions on Backups

Check failure on line 1 in .jules/sentinel.md

View workflow job for this annotation

GitHub Actions / Lint Documentation

First line in a file should be a top-level heading 

.jules/sentinel.md:1 MD041/first-line-heading/first-line-h1 First line in a file should be a top-level heading [Context: "## 2026-02-22 - Insecure Defau..."] https://github.com/DavidAnson/markdownlint/blob/v0.34.0/doc/md041.md

Check failure on line 1 in .jules/sentinel.md

View workflow job for this annotation

GitHub Actions / Lint Documentation

Headings should be surrounded by blank lines 

.jules/sentinel.md:1 MD022/blanks-around-headings Headings should be surrounded by blank lines [Expected: 1; Actual: 0; Below] [Context: "## 2026-02-22 - Insecure Default Permissions on Backups"] https://github.com/DavidAnson/markdownlint/blob/v0.34.0/doc/md022.md
**Vulnerability:** The `tools/backup-projects.sh` script created zip archives with default permissions (often `644` or `664`), allowing other users on the system to read potentially sensitive project backups.

Check failure on line 2 in .jules/sentinel.md

View workflow job for this annotation

GitHub Actions / Lint Documentation

Line length 

.jules/sentinel.md:2:81 MD013/line-length Line length [Expected: 80; Actual: 208] https://github.com/DavidAnson/markdownlint/blob/v0.34.0/doc/md013.md
**Learning:** Shell scripts using tools like `zip` or `tar` do not automatically restrict permissions of the output file unless `umask` is set.

Check failure on line 3 in .jules/sentinel.md

View workflow job for this annotation

GitHub Actions / Lint Documentation

Line length 

.jules/sentinel.md:3:81 MD013/line-length Line length [Expected: 80; Actual: 143] https://github.com/DavidAnson/markdownlint/blob/v0.34.0/doc/md013.md
**Prevention:** Always set `umask 077` at the beginning of shell scripts that generate sensitive files or directories to ensure they are private by default.

Check failure on line 4 in .jules/sentinel.md

View workflow job for this annotation

GitHub Actions / Lint Documentation

Line length 

.jules/sentinel.md:4:81 MD013/line-length Line length [Expected: 80; Actual: 156] https://github.com/DavidAnson/markdownlint/blob/v0.34.0/doc/md013.md
Comment on lines +1 to +4
Copy link

@coderabbitai coderabbitai bot Feb 22, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

⚠️ Potential issue | 🟡 Minor

Fix markdown lint failures blocking the CI "Lint Documentation" check.

Five violations are reported:

  • MD041 (line 1): First line must be a top-level # heading, not ##.
  • MD022 (line 1): Heading must be surrounded by blank lines.
  • MD013 (lines 2–4): Lines exceed the 80-character limit.
📝 Proposed fix 
-## 2026-02-22 - Insecure Default Permissions on Backups
-**Vulnerability:** The `tools/backup-projects.sh` script created zip archives with default permissions (often `644` or `664`), allowing other users on the system to read potentially sensitive project backups.
-**Learning:** Shell scripts using tools like `zip` or `tar` do not automatically restrict permissions of the output file unless `umask` is set.
-**Prevention:** Always set `umask 077` at the beginning of shell scripts that generate sensitive files or directories to ensure they are private by default.
+# Security Bulletins
+
+## 2026-02-22 - Insecure Default Permissions on Backups
+
+**Vulnerability:** `tools/backup-projects.sh` created zip archives with default
+permissions (often `644` or `664`), allowing other users to read sensitive
+project backups.
+
+**Learning:** Shell scripts using `zip` or `tar` do not restrict output file
+permissions unless `umask` is set.
+
+**Prevention:** Set `umask 077` at the start of scripts that generate sensitive
+files or directories to ensure owner-only permissions by default.
📝 Committable suggestion

‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.

Suggested change
## 2026-02-22 - Insecure Default Permissions on Backups
**Vulnerability:** The `tools/backup-projects.sh` script created zip archives with default permissions (often `644` or `664`), allowing other users on the system to read potentially sensitive project backups.
**Learning:** Shell scripts using tools like `zip` or `tar` do not automatically restrict permissions of the output file unless `umask` is set.
**Prevention:** Always set `umask 077` at the beginning of shell scripts that generate sensitive files or directories to ensure they are private by default.
# Security Bulletins
## 2026-02-22 - Insecure Default Permissions on Backups
**Vulnerability:** `tools/backup-projects.sh` created zip archives with default
permissions (often `644` or `664`), allowing other users to read sensitive
project backups.
**Learning:** Shell scripts using `zip` or `tar` do not restrict output file
permissions unless `umask` is set.
**Prevention:** Set `umask 077` at the start of scripts that generate sensitive
files or directories to ensure owner-only permissions by default.
🧰 Tools
🪛 GitHub Check: Lint Documentation

[failure] 4-4: Line length
.jules/sentinel.md:4:81 MD013/line-length Line length [Expected: 80; Actual: 156] https://github.com/DavidAnson/markdownlint/blob/v0.34.0/doc/md013.md

[failure] 3-3: Line length
.jules/sentinel.md:3:81 MD013/line-length Line length [Expected: 80; Actual: 143] https://github.com/DavidAnson/markdownlint/blob/v0.34.0/doc/md013.md

[failure] 2-2: Line length
.jules/sentinel.md:2:81 MD013/line-length Line length [Expected: 80; Actual: 208] https://github.com/DavidAnson/markdownlint/blob/v0.34.0/doc/md013.md

[failure] 1-1: First line in a file should be a top-level heading
.jules/sentinel.md:1 MD041/first-line-heading/first-line-h1 First line in a file should be a top-level heading [Context: "## 2026-02-22 - Insecure Defau..."] https://github.com/DavidAnson/markdownlint/blob/v0.34.0/doc/md041.md

[failure] 1-1: Headings should be surrounded by blank lines
.jules/sentinel.md:1 MD022/blanks-around-headings Headings should be surrounded by blank lines [Expected: 1; Actual: 0; Below] [Context: "## 2026-02-22 - Insecure Default Permissions on Backups"] https://github.com/DavidAnson/markdownlint/blob/v0.34.0/doc/md022.md

🤖 Prompt for AI Agents 
Verify each finding against the current code and only fix it if needed.

In @.jules/sentinel.md around lines 1 - 4, Change the top-level heading to a
single `#` and ensure it's surrounded by a blank line, and wrap the description
lines to <=80 characters to satisfy MD041, MD022 and MD013; specifically edit
.jules/sentinel.md to replace the `## 2026-02-22 - Insecure Default Permissions
on Backups` line with `# 2026-02-22 - Insecure Default Permissions on Backups`,
add a blank line before and after that heading, and break the subsequent
sentence lines (the Vulnerability, Learning, and Prevention lines referencing
tools/backup-projects.sh and umask 077) into shorter lines so each stays under
the 80-character limit.

55 changes: 55 additions & 0 deletions tests/verify_backup_permissions.sh
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,55 @@
#!/bin/bash

Comment on lines +1 to +2
Copy link

@coderabbitai coderabbitai bot Feb 22, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

⚠️ Potential issue | 🟠 Major

Add set -euo pipefail to prevent silent failures from producing misleading test results.

Without strict mode, a failing intermediate command (e.g., stat encountering a missing file, or an unset variable expanding to empty) leaves $PERMS empty or wrong, causing the final comparison to report a false failure — or worse, a false pass — with no indication of what went wrong.

♻️ Proposed fix 
 #!/bin/bash
+set -euo pipefail
+
 # Setup test environment
🤖 Prompt for AI Agents 
Verify each finding against the current code and only fix it if needed.

In `@tests/verify_backup_permissions.sh` around lines 1 - 2, Add strict-mode to
the shell script to prevent silent failures: insert "set -euo pipefail" after
the shebang so that failures (e.g., stat errors or unset variables like PERMS)
cause the script to exit with clear errors instead of producing empty/wrong
PERMS or misleading test results; ensure any use of PERMS and other variables
are handled with proper quoting to satisfy -u.

# Setup test environment
PROJECT_DIR="$HOME/kidchenko"
BACKUP_DIR="${XDG_DATA_HOME:-$HOME/.local/share}/dotfiles/backups"
BACKUP_DIR="${BACKUP_DIR/#\~/$HOME}" # Expand ~ just in case

# Create dummy project
mkdir -p "$PROJECT_DIR"
echo "secret content" > "$PROJECT_DIR/secret.txt"

# Ensure cleanup
trap 'rm -rf "$PROJECT_DIR"' EXIT
Comment on lines +9 to +13
Copy link

@coderabbitai coderabbitai bot Feb 22, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

⚠️ Potential issue | 🔴 Critical

trap 'rm -rf "$PROJECT_DIR"' EXIT will unconditionally destroy ~/kidchenko if it exists with real data.

PROJECT_DIR is set to $HOME/kidchenko — a directory that likely contains the owner's real projects. mkdir -p silently succeeds when the directory already exists, so the trap is armed and fires on every exit, deleting everything. Any developer running this test locally on their dotfiles machine loses all their project data.

Fix: check whether the directory pre-existed and only delete it if this test created it.

🛡️ Proposed fix 
-# Create dummy project
-mkdir -p "$PROJECT_DIR"
-echo "secret content" > "$PROJECT_DIR/secret.txt"
-
-# Ensure cleanup
-trap 'rm -rf "$PROJECT_DIR"' EXIT
+# Create dummy project – track pre-existence to avoid destroying real data
+_project_dir_existed=false
+[[ -d "$PROJECT_DIR" ]] && _project_dir_existed=true
+mkdir -p "$PROJECT_DIR"
+echo "secret content" > "$PROJECT_DIR/secret.txt"
+
+# Cleanup: only remove the directory wholesale if this test created it
+_cleanup() {
+  rm -f "$PROJECT_DIR/secret.txt"
+  [[ "$_project_dir_existed" == false ]] && rm -rf "$PROJECT_DIR"
+}
+trap _cleanup EXIT
📝 Committable suggestion

‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.

Suggested change
mkdir -p "$PROJECT_DIR"
echo "secret content" > "$PROJECT_DIR/secret.txt"
# Ensure cleanup
trap 'rm -rf "$PROJECT_DIR"' EXIT
# Create dummy project – track pre-existence to avoid destroying real data
_project_dir_existed=false
[[ -d "$PROJECT_DIR" ]] && _project_dir_existed=true
mkdir -p "$PROJECT_DIR"
echo "secret content" > "$PROJECT_DIR/secret.txt"
# Cleanup: only remove the directory wholesale if this test created it
_cleanup() {
rm -f "$PROJECT_DIR/secret.txt"
[[ "$_project_dir_existed" == false ]] && rm -rf "$PROJECT_DIR"
}
trap _cleanup EXIT
🤖 Prompt for AI Agents 
Verify each finding against the current code and only fix it if needed.

In `@tests/verify_backup_permissions.sh` around lines 9 - 13, The trap currently
unconditionally removes "$PROJECT_DIR" on EXIT which will delete an existing
$HOME/kidchenko; modify the setup to detect whether PROJECT_DIR already existed
(e.g., check [ -d "$PROJECT_DIR" ] before creating, set a flag like
PROJECT_DIR_PREEXISTED or PROJECT_DIR_CREATED), only create the directory if
missing (mkdir -p as now) and update the trap to remove "$PROJECT_DIR" on EXIT
only when the flag indicates the test created it; use PROJECT_DIR and the trap
command references to locate where to add the existence check and conditional
cleanup.


# Run backup script
# It reads default folders which includes ~/kidchenko
echo "Running backup script..."
# Using --verbose to see output, but suppressing standard output unless needed
if ! bash tools/backup-projects.sh backup --verbose > /tmp/backup_output.log 2>&1; then
echo "Backup failed. Output:"
cat /tmp/backup_output.log
exit 1
fi

# Find the latest backup
LATEST_BACKUP=$(ls -t "$BACKUP_DIR"/project-backup-*.zip 2>/dev/null | head -n1)

if [[ -z "$LATEST_BACKUP" ]]; then
echo "Error: No backup file created in $BACKUP_DIR"
cat /tmp/backup_output.log
exit 1
fi

echo "Backup created: $LATEST_BACKUP"

# Check permissions
if [[ "$OSTYPE" == "darwin"* ]]; then
PERMS=$(stat -f %Lp "$LATEST_BACKUP")
else
PERMS=$(stat -c %a "$LATEST_BACKUP")
fi

echo "Permissions: $PERMS"

# Cleanup backup file
rm -f "$LATEST_BACKUP"

# We expect 600 (rw-------)
if [[ "$PERMS" != "600" ]]; then
echo "FAILURE: Permissions are too open ($PERMS). Expected 600."
exit 1
else
echo "SUCCESS: Permissions are correct (600)."
exit 0
fi
3 changes: 3 additions & 0 deletions tools/backup-projects.sh
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -27,6 +27,9 @@
# Pipestatus
set -o pipefail

# Security: Ensure all created files/directories are private (read/write only by owner)
umask 077

# --- Configuration ---
CONFIG_FILE="${XDG_CONFIG_HOME:-$HOME/.config}/dotfiles/config.yaml"
LOG_DIR="${XDG_STATE_HOME:-$HOME/.local/state}/dotfiles"
Expand Down
Loading