Skip to content

🛡️ Sentinel: [MEDIUM] Secure backup file permissions #36

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] Secure backup file permissions #36

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 @@
## 2024-05-24 - Secure Backup Permissions

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: "## 2024-05-24 - Secure Backup ..."] 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: "## 2024-05-24 - Secure Backup Permissions"] https://github.com/DavidAnson/markdownlint/blob/v0.34.0/doc/md022.md
**Vulnerability:** Backup archives created by `zip` without explicit permissions were world-readable (depending on umask), exposing sensitive project data.

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: 155] https://github.com/DavidAnson/markdownlint/blob/v0.34.0/doc/md013.md
**Learning:** `mkdir -p` and `zip` respect the process `umask`, but relying on system defaults is insecure for sensitive data.

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: 126] https://github.com/DavidAnson/markdownlint/blob/v0.34.0/doc/md013.md
**Prevention:** Explicitly set `umask 077` at the start of sensitive operations and use `chmod` to enforce restrictive permissions (600/700) on critical artifacts.

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: 163] https://github.com/DavidAnson/markdownlint/blob/v0.34.0/doc/md013.md
Comment on lines +1 to +4
Copy link

@coderabbitai coderabbitai bot Feb 20, 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 5 markdownlint violations failing CI.

The Lint Documentation check is failing with five violations:

Line Rule Issue
1 MD041 First heading must be # (H1), not ## (H2)
1 MD022 Heading must be followed by a blank line
2 MD013 Line length 155 > 80
3 MD013 Line length 126 > 80
4 MD013 Line length 163 > 80
📝 Proposed fix 
-## 2024-05-24 - Secure Backup Permissions
-**Vulnerability:** Backup archives created by `zip` without explicit permissions were world-readable (depending on umask), exposing sensitive project data.
-**Learning:** `mkdir -p` and `zip` respect the process `umask`, but relying on system defaults is insecure for sensitive data.
-**Prevention:** Explicitly set `umask 077` at the start of sensitive operations and use `chmod` to enforce restrictive permissions (600/700) on critical artifacts.
+# 2024-05-24 - Secure Backup Permissions
+
+**Vulnerability:** Backup archives created by `zip` without explicit permissions were world-readable
+(depending on umask), exposing sensitive project data.
+
+**Learning:** `mkdir -p` and `zip` respect the process `umask`, but relying on system defaults is
+insecure for sensitive data.
+
+**Prevention:** Explicitly set `umask 077` at the start of sensitive operations and use `chmod` to
+enforce restrictive permissions (600/700) on critical artifacts.
📝 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
## 2024-05-24 - Secure Backup Permissions
**Vulnerability:** Backup archives created by `zip` without explicit permissions were world-readable (depending on umask), exposing sensitive project data.
**Learning:** `mkdir -p` and `zip` respect the process `umask`, but relying on system defaults is insecure for sensitive data.
**Prevention:** Explicitly set `umask 077` at the start of sensitive operations and use `chmod` to enforce restrictive permissions (600/700) on critical artifacts.
# 2024-05-24 - Secure Backup Permissions
**Vulnerability:** Backup archives created by `zip` without explicit permissions were world-readable
(depending on umask), exposing sensitive project data.
**Learning:** `mkdir -p` and `zip` respect the process `umask`, but relying on system defaults is
insecure for sensitive data.
**Prevention:** Explicitly set `umask 077` at the start of sensitive operations and use `chmod` to
enforce restrictive permissions (600/700) on critical artifacts.
🧰 Tools
🪛 GitHub Check: Lint Documentation

[failure] 4-4: Line length
.jules/sentinel.md:4:81 MD013/line-length Line length [Expected: 80; Actual: 163] 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: 126] 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: 155] 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: "## 2024-05-24 - Secure Backup ..."] 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: "## 2024-05-24 - Secure Backup Permissions"] 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, The markdown fails markdownlint:
change the top-level heading from "## 2024-05-24 - Secure Backup Permissions" to
a H1 ("# 2024-05-24 - Secure Backup Permissions") and add a blank line after
that heading (fixes MD041 and MD022); then wrap or reflow the long description
lines (the lines starting with "**Vulnerability:**", "**Learning:**", and
"**Prevention:**") so each line is <=80 characters (break into multiple
sentences/lines while preserving content and bullets), and ensure any
newly-wrapped lines maintain Markdown formatting (keep the bold labels like
**Vulnerability:** intact) to resolve the MD013 violations.

73 changes: 73 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,73 @@
#!/bin/bash
set -e

# Setup test paths
TEST_DIR="$(mktemp -d)"
TEST_SOURCE="$TEST_DIR/source"
TEST_BACKUP_DIR="$TEST_DIR/backups"
TEST_CONFIG="$TEST_DIR/config.yaml"
SCRIPT_PATH="$(pwd)/tools/backup-projects.sh"

# Cleanup on exit
trap 'rm -rf "$TEST_DIR"' EXIT

# Create source directory with a file
mkdir -p "$TEST_SOURCE"
echo "sensitive data" > "$TEST_SOURCE/secret.txt"

# Create config file
cat > "$TEST_CONFIG" <<EOF
backup:
folders:
- $TEST_SOURCE
local:
base_dir: $TEST_BACKUP_DIR
retention_days: 1
remote:
enabled: false
logging:
enabled: false
EOF
Comment on lines +19 to +30
Copy link

@coderabbitai coderabbitai bot Feb 20, 2026

Choose a reason for hiding this comment

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

⚠️ Potential issue | 🟠 Major

Test isolation broken: backup script writes git-repos.log outside TEST_DIR.

The test config sets logging.enabled: false but omits logging.dir. As a result, LOG_DIR defaults to ${XDG_STATE_HOME:-$HOME/.local/state}/dotfiles, which is outside TEST_DIR. Both cmd_backup and sync_git_repos unconditionally call mkdir -p "$LOG_DIR" and write $LOG_DIR/git-repos.log regardless of the LOGGING_ENABLED flag — those files land in the user's home directory and are not cleaned up by the trap. On a system where $HOME/.local/state/dotfiles doesn't yet exist, this also creates it with mode 700 (due to umask 077), potentially altering that directory's permissions permanently.

Add logging.dir to the test config to contain all I/O within TEST_DIR:

🐛 Proposed fix 
 cat > "$TEST_CONFIG" <<EOF
 backup:
   folders:
     - $TEST_SOURCE
   local:
     base_dir: $TEST_BACKUP_DIR
     retention_days: 1
   remote:
     enabled: false
   logging:
     enabled: false
+    dir: $TEST_DIR/state
 EOF
📝 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
cat > "$TEST_CONFIG" <<EOF
backup:
folders:
- $TEST_SOURCE
local:
base_dir: $TEST_BACKUP_DIR
retention_days: 1
remote:
enabled: false
logging:
enabled: false
EOF
cat > "$TEST_CONFIG" <<EOF
backup:
folders:
- $TEST_SOURCE
local:
base_dir: $TEST_BACKUP_DIR
retention_days: 1
remote:
enabled: false
logging:
enabled: false
dir: $TEST_DIR/state
EOF
🤖 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 19 - 30, The test writes
logs outside TEST_DIR because the generated TEST_CONFIG lacks logging.dir, so
LOG_DIR falls back to XDG_STATE_HOME and cmd_backup/sync_git_repos create
$LOG_DIR/git-repos.log unconditionally; update the test config written to
TEST_CONFIG to include a logging.dir set inside TEST_DIR (e.g.,
"${TEST_DIR}/state" or similar) so that LOG_DIR resolves inside TEST_DIR and all
mkdir -p and log writes performed by cmd_backup and sync_git_repos are contained
and cleaned up by the trap.


# Make sure script is executable
chmod +x "$SCRIPT_PATH"

# Run backup script
"$SCRIPT_PATH" backup --config "$TEST_CONFIG" >/dev/null

# Check backup file permissions
BACKUP_FILE=$(find "$TEST_BACKUP_DIR" -name "*.zip" | head -n 1)

if [[ -z "$BACKUP_FILE" ]]; then
echo "Backup file not found!"
exit 1
fi

if [[ "$(uname -s)" == "Darwin" ]]; then
PERMS=$(stat -f "%Lp" "$BACKUP_FILE")
DIR_PERMS=$(stat -f "%Lp" "$TEST_BACKUP_DIR")
else
PERMS=$(stat -c "%a" "$BACKUP_FILE")
DIR_PERMS=$(stat -c "%a" "$TEST_BACKUP_DIR")
fi
echo "Backup file permissions: $PERMS"
echo "Backup directory permissions: $DIR_PERMS"

FAILED=0

# Verify if permissions are secure (600 for file, 700 for directory)
if [[ "$PERMS" != "600" ]]; then
echo "FAIL: Backup file permissions are not 600"
FAILED=1
else
echo "PASS: Backup file permissions are 600"
fi

if [[ "$DIR_PERMS" != "700" ]]; then
echo "FAIL: Backup directory permissions are not 700"
FAILED=1
else
echo "PASS: Backup directory permissions are 700"
fi

exit $FAILED
6 changes: 6 additions & 0 deletions tools/backup-projects.sh
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -350,7 +350,10 @@ cmd_backup() {

# Setup directories
if [[ "$DRY_RUN" != true ]]; then
# Ensure secure permissions for backup directory (700)
umask 077
mkdir -p "$BACKUP_TEMP_DIR"
chmod 700 "$BACKUP_TEMP_DIR"
mkdir -p "$LOG_DIR"
else
debug "Would create: $BACKUP_TEMP_DIR"
Expand Down Expand Up @@ -425,6 +428,9 @@ cmd_backup() {
return 1
fi

# Ensure secure permissions for backup archive (600)
chmod 600 "$archive_path"
Comment on lines +431 to +432
Copy link

@coderabbitai coderabbitai bot Feb 20, 2026

Choose a reason for hiding this comment

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

⚠️ Potential issue | 🟡 Minor

chmod 600 failure is silently swallowed.

The script sets set -o pipefail but not set -e, so a failing chmod (e.g., read-only filesystem, permission denied) goes undetected and the run reports success with a potentially mis-permissioned archive. Add explicit error handling:

🛡️ Proposed fix 
-        # Ensure secure permissions for backup archive (600)
-        chmod 600 "$archive_path"
+        # Ensure secure permissions for backup archive (600)
+        if ! chmod 600 "$archive_path"; then
+            error "Failed to set secure permissions on archive: $archive_path"
+            return 1
+        fi
📝 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
# Ensure secure permissions for backup archive (600)
chmod 600 "$archive_path"
# Ensure secure permissions for backup archive (600)
if ! chmod 600 "$archive_path"; then
error "Failed to set secure permissions on archive: $archive_path"
return 1
fi
🤖 Prompt for AI Agents 
Verify each finding against the current code and only fix it if needed.

In `@tools/backup-projects.sh` around lines 431 - 432, The chmod 600
"$archive_path" call can fail silently because the script doesn't enable
errexit; update the script so the failure is detected and causes the job to
fail: either enable shell errexit (set -e) near the top alongside set -o
pipefail, or immediately check the exit status of chmod 600 "$archive_path" (or
append an explicit failure handler) and log an error referencing $archive_path
then exit with non-zero; locate the chmod call and implement one of these two
fixes so permission errors aren't swallowed.


local archive_size
archive_size=$(du -h "$archive_path" | cut -f1)
say "Archive created: $archive_size"
Expand Down
Loading