Skip to content

fix: guard cliproxyapi auth on restart #545

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

Merged
shunkakinoki merged 1 commit into from
Jan 11, 2026
Merged

fix: guard cliproxyapi auth on restart #545

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
2 changes: 1 addition & 1 deletion .github/workflows/docker.yml
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -101,7 +101,7 @@ jobs:
timeout-minutes: 30
steps:
- name: Download digests
uses: actions/download-artifact@v7
uses: actions/download-artifact@v6
with:
path: ${{ runner.temp }}/digests
pattern: digests-*
Expand Down
1 change: 1 addition & 0 deletions .github/workflows/nix.yml
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -93,6 +93,7 @@ jobs:
- nix-flake
- nix-format
- nix-linux
- nix-nixos
runs-on: ubuntu-latest
timeout-minutes: 3
steps:
Expand Down
1 change: 1 addition & 0 deletions home-manager/services/cliproxyapi/default.nix
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -13,6 +13,7 @@ let

startScript = pkgs.replaceVars ./scripts/start.sh {
sed = "${pkgs.gnused}/bin/sed";
aws = "${pkgs.awscli2}/bin/aws";
};

dockerStartScript = pkgs.writeShellScript "cliproxyapi-docker-start" ''
Expand Down
42 changes: 42 additions & 0 deletions home-manager/services/cliproxyapi/scripts/start.sh
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -6,6 +6,7 @@ CONFIG_DIR="${HOME}/.cli-proxy-api"
TEMPLATE="$CONFIG_DIR/config.template.yaml"
CONFIG="$CONFIG_DIR/config.yaml"
ENV_FILE="${HOME}/dotfiles/.env"
AUTH_DIR="${CONFIG_DIR}/objectstore/auths"

if [ -f "$ENV_FILE" ]; then
set -a
Expand All @@ -26,6 +27,47 @@ OBJECTSTORE_SECRET_KEY="$(strip_quotes "${OBJECTSTORE_SECRET_KEY:-}")"
MANAGEMENT_PASSWORD="${CLIPROXY_MANAGEMENT_PASSWORD:-}"
export OBJECTSTORE_ENDPOINT OBJECTSTORE_BUCKET OBJECTSTORE_ACCESS_KEY OBJECTSTORE_SECRET_KEY MANAGEMENT_PASSWORD

if [ -n "$OBJECTSTORE_ENDPOINT" ] && [ -n "$OBJECTSTORE_ACCESS_KEY" ] && [ -n "$OBJECTSTORE_SECRET_KEY" ]; then

@mesa-dot-dev mesa-dot-dev Bot Jan 11, 2026

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

Medium

If multiple instances of this service start simultaneously (e.g., during a deployment rollout), they could all detect an empty AUTH_DIR and race to sync from/to S3, potentially causing conflicts or data corruption. Consider adding a lock mechanism or using S3's conditional operations to prevent concurrent modifications.

Agent: 🏛 Architecture • Fix in Cursor • Fix in Claude

Prompt for Agent 
Task: Address review feedback left on GitHub.
Repository: shunkakinoki/dotfiles#545
File: home-manager/services/cliproxyapi/scripts/start.sh#L30
Action: Open this file location in your editor, inspect the highlighted code, and resolve the issue described below.

Feedback:
If multiple instances of this service start simultaneously (e.g., during a deployment rollout), they could all detect an empty AUTH_DIR and race to sync from/to S3, potentially causing conflicts or data corruption. Consider adding a lock mechanism or using S3's conditional operations to prevent concurrent modifications.

Copilot AI Jan 11, 2026

Copy link

Choose a reason for hiding this comment

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

The condition should also check that OBJECTSTORE_BUCKET is not empty. While it defaults to "cliproxyapi" on line 24, it's possible for the variable to be explicitly set to an empty string in the environment, which would result in invalid S3 paths like "s3:///auths/".

Suggested change
if [ -n "$OBJECTSTORE_ENDPOINT" ] && [ -n "$OBJECTSTORE_ACCESS_KEY" ] && [ -n "$OBJECTSTORE_SECRET_KEY" ]; then
if [ -n "$OBJECTSTORE_ENDPOINT" ] && [ -n "$OBJECTSTORE_ACCESS_KEY" ] && [ -n "$OBJECTSTORE_SECRET_KEY" ] && [ -n "$OBJECTSTORE_BUCKET" ]; then

Copilot uses AI. Check for mistakes.
mkdir -p "$AUTH_DIR"

@cubic-dev-ai cubic-dev-ai Bot Jan 11, 2026 •
edited
Loading

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

P2: Auth cache directory is created/populated without restrictive permissions, leaving OAuth tokens potentially world-readable

Prompt for AI agents 
Check if this issue is valid — if so, understand the root cause and fix it. At home-manager/services/cliproxyapi/scripts/start.sh, line 31:

<comment>Auth cache directory is created/populated without restrictive permissions, leaving OAuth tokens potentially world-readable</comment>

<file context>
@@ -26,6 +27,47 @@ OBJECTSTORE_SECRET_KEY="$(strip_quotes "${OBJECTSTORE_SECRET_KEY:-}")"
 export OBJECTSTORE_ENDPOINT OBJECTSTORE_BUCKET OBJECTSTORE_ACCESS_KEY OBJECTSTORE_SECRET_KEY MANAGEMENT_PASSWORD
 
+if [ -n "$OBJECTSTORE_ENDPOINT" ] && [ -n "$OBJECTSTORE_ACCESS_KEY" ] && [ -n "$OBJECTSTORE_SECRET_KEY" ]; then
+  mkdir -p "$AUTH_DIR"
+
+  if [ -z "$(ls -A "$AUTH_DIR" 2>/dev/null)" ]; then
</file context>
Fix with Cubic


if [ -z "$(ls -A "$AUTH_DIR" 2>/dev/null)" ]; then
echo "⚠️ Local auth cache empty; hydrating from S3" >&2
AWS_ACCESS_KEY_ID="$OBJECTSTORE_ACCESS_KEY" \
AWS_SECRET_ACCESS_KEY="$OBJECTSTORE_SECRET_KEY" \
@aws@ s3 sync \
--endpoint-url="$OBJECTSTORE_ENDPOINT" \
--no-progress \
"s3://${OBJECTSTORE_BUCKET}/auths/" \
"$AUTH_DIR/" || true

@mesa-dot-dev mesa-dot-dev Bot Jan 11, 2026

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

Medium

The || true here silently ignores all S3 sync failures, including authentication errors, network issues, or misconfigured endpoints. Consider logging the error and potentially failing fast if S3 credentials are provided but the sync fails, as this could indicate a configuration problem that will persist.

Agent: 🏛 Architecture • Fix in Cursor • Fix in Claude

Prompt for Agent 
Task: Address review feedback left on GitHub.
Repository: shunkakinoki/dotfiles#545
File: home-manager/services/cliproxyapi/scripts/start.sh#L41
Action: Open this file location in your editor, inspect the highlighted code, and resolve the issue described below.

Feedback:
The `|| true` here silently ignores all S3 sync failures, including authentication errors, network issues, or misconfigured endpoints. Consider logging the error and potentially failing fast if S3 credentials are provided but the sync fails, as this could indicate a configuration problem that will persist.


AWS_ACCESS_KEY_ID="$OBJECTSTORE_ACCESS_KEY" \
AWS_SECRET_ACCESS_KEY="$OBJECTSTORE_SECRET_KEY" \
@aws@ s3 sync \
--endpoint-url="$OBJECTSTORE_ENDPOINT" \
--no-progress \
"s3://${OBJECTSTORE_BUCKET}/backup/auths/" \
"$AUTH_DIR/" || true
fi
Comment on lines +33 to +50

Copilot AI Jan 11, 2026

Copy link

Choose a reason for hiding this comment

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

This duplicates the hydration logic already present in hydrate.sh (lines 33-47). This creates maintenance burden as changes to the sync logic need to be made in multiple places. Consider calling the existing hydrate.sh script instead of duplicating the logic, or extract this into a shared function.

Copilot uses AI. Check for mistakes.

if [ -n "$(ls -A "$AUTH_DIR" 2>/dev/null)" ]; then

@mesa-dot-dev mesa-dot-dev Bot Jan 11, 2026

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

Medium

This condition checks if AUTH_DIR has files after the hydration attempts, but both hydration commands used || true (lines 41, 49). If both S3 syncs failed silently, this block will still execute and sync an empty directory back to S3, potentially overwriting valid remote data. Consider checking the exit status of the hydration commands before proceeding with the re-sync.

Agent: 🏛 Architecture • Fix in Cursor • Fix in Claude

Prompt for Agent 
Task: Address review feedback left on GitHub.
Repository: shunkakinoki/dotfiles#545
File: home-manager/services/cliproxyapi/scripts/start.sh#L52
Action: Open this file location in your editor, inspect the highlighted code, and resolve the issue described below.

Feedback:
This condition checks if AUTH_DIR has files after the hydration attempts, but both hydration commands used `|| true` (lines 41, 49). If both S3 syncs failed silently, this block will still execute and sync an empty directory back to S3, potentially overwriting valid remote data. Consider checking the exit status of the hydration commands before proceeding with the re-sync.

AWS_ACCESS_KEY_ID="$OBJECTSTORE_ACCESS_KEY" \
AWS_SECRET_ACCESS_KEY="$OBJECTSTORE_SECRET_KEY" \
@aws@ s3 sync \
--endpoint-url="$OBJECTSTORE_ENDPOINT" \
--no-progress \
"$AUTH_DIR/" \
"s3://${OBJECTSTORE_BUCKET}/auths/" || true

AWS_ACCESS_KEY_ID="$OBJECTSTORE_ACCESS_KEY" \
AWS_SECRET_ACCESS_KEY="$OBJECTSTORE_SECRET_KEY" \
@aws@ s3 sync \
--endpoint-url="$OBJECTSTORE_ENDPOINT" \
--no-progress \
"$AUTH_DIR/" \
"s3://${OBJECTSTORE_BUCKET}/backup/auths/" || true
Comment on lines +53 to +67

Copilot AI Jan 11, 2026

Copy link

Choose a reason for hiding this comment

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

This duplicates the backup logic already present in backup.sh (lines 38-52). This creates maintenance burden as changes to the sync logic need to be made in multiple places. Consider calling the existing backup.sh script instead of duplicating the logic, or extract this into a shared function.

Suggested change
AWS_ACCESS_KEY_ID="$OBJECTSTORE_ACCESS_KEY" \
AWS_SECRET_ACCESS_KEY="$OBJECTSTORE_SECRET_KEY" \
@aws@ s3 sync \
--endpoint-url="$OBJECTSTORE_ENDPOINT" \
--no-progress \
"$AUTH_DIR/" \
"s3://${OBJECTSTORE_BUCKET}/auths/" || true
AWS_ACCESS_KEY_ID="$OBJECTSTORE_ACCESS_KEY" \
AWS_SECRET_ACCESS_KEY="$OBJECTSTORE_SECRET_KEY" \
@aws@ s3 sync \
--endpoint-url="$OBJECTSTORE_ENDPOINT" \
--no-progress \
"$AUTH_DIR/" \
"s3://${OBJECTSTORE_BUCKET}/backup/auths/" || true
script_dir="$(cd "$(dirname "${BASH_SOURCE[0]:-$0}")" && pwd)"
backup_script="${script_dir}/backup.sh"
if [ ! -x "$backup_script" ]; then
echo "Expected backup script not found or not executable: $backup_script" >&2
exit 1
fi
OBJECTSTORE_ENDPOINT="$OBJECTSTORE_ENDPOINT" \
OBJECTSTORE_BUCKET="$OBJECTSTORE_BUCKET" \
OBJECTSTORE_ACCESS_KEY="$OBJECTSTORE_ACCESS_KEY" \
OBJECTSTORE_SECRET_KEY="$OBJECTSTORE_SECRET_KEY" \
AUTH_DIR="$AUTH_DIR" \
"$backup_script"

Copilot uses AI. Check for mistakes.
Comment on lines +37 to +67

Copilot AI Jan 11, 2026

Copy link

Choose a reason for hiding this comment

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

The || true at the end of each aws s3 sync command ensures the script continues even if syncs fail. While this provides resilience, failed syncs are completely silent with no error logging. Consider adding error logging after failed syncs to aid debugging, for example: || echo "⚠️ Failed to sync from/to S3 [location]" >&2

Copilot uses AI. Check for mistakes.
fi
Comment on lines +52 to +68

Copilot AI Jan 11, 2026

Copy link

Choose a reason for hiding this comment

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

On every restart when the auth cache is not empty, four S3 sync operations are performed (lines 55-67). This happens even if the local and remote are already in sync. The aws s3 sync command is efficient and only uploads changed files, but the overhead of establishing connections and checking every file on each restart could add startup latency. Consider adding a timestamp check or other mechanism to skip syncing if a recent sync has already occurred.

Copilot uses AI. Check for mistakes.
fi
Comment on lines +30 to +69

@gemini-code-assist gemini-code-assist Bot Jan 11, 2026

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

medium

The new logic for S3 synchronization contains significant code duplication, especially for the aws s3 sync commands and the AWS credentials. This can be refactored to improve readability and maintainability.

I suggest defining a helper function for the s3 sync operation and exporting the AWS credentials once at the beginning of the block. This will make the script cleaner, less error-prone, and easier to understand.

if [ -n "$OBJECTSTORE_ENDPOINT" ] && [ -n "$OBJECTSTORE_ACCESS_KEY" ] && [ -n "$OBJECTSTORE_SECRET_KEY" ]; then
  export AWS_ACCESS_KEY_ID="$OBJECTSTORE_ACCESS_KEY"
  export AWS_SECRET_ACCESS_KEY="$OBJECTSTORE_SECRET_KEY"

  s3_sync() {
    @aws@ s3 sync --endpoint-url="$OBJECTSTORE_ENDPOINT" --no-progress "$@" || true
  }

  mkdir -p "$AUTH_DIR"

  if [ -z "$(ls -A "$AUTH_DIR" 2>/dev/null)" ]; then
    echo "⚠️  Local auth cache empty; hydrating from S3" >&2
    s3_sync "s3://${OBJECTSTORE_BUCKET}/auths/" "$AUTH_DIR/"
    s3_sync "s3://${OBJECTSTORE_BUCKET}/backup/auths/" "$AUTH_DIR/"
  fi

  if [ -n "$(ls -A "$AUTH_DIR" 2>/dev/null)" ]; then
    s3_sync "$AUTH_DIR/" "s3://${OBJECTSTORE_BUCKET}/auths/"
    s3_sync "$AUTH_DIR/" "s3://${OBJECTSTORE_BUCKET}/backup/auths/"
  fi
fi

Comment on lines +30 to +69

Copilot AI Jan 11, 2026

Copy link

Choose a reason for hiding this comment

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

The new auth guard logic (lines 30-69) lacks test coverage. While hydrate.sh and backup.sh have tests in spec/cliproxyapi_backup_spec.sh, the conditional logic for checking if AUTH_DIR is empty, hydrating, and syncing back should be tested. Consider adding test cases to spec/cliproxyapi_spec.sh that verify: 1) hydration occurs when AUTH_DIR is empty, 2) hydration is skipped when AUTH_DIR has files, 3) sync back occurs after hydration, and 4) proper handling when S3 credentials are missing.

Copilot uses AI. Check for mistakes.

# Generate config from template
if [ -f "$TEMPLATE" ]; then
@sed@ \
Expand Down
Loading