Skip to content

Fix custom image tag handling in installer smoke#216

Merged
lroolle merged 1 commit intomainfrom
fix/installer-tag
Mar 12, 2026
Merged

Fix custom image tag handling in installer smoke#216
lroolle merged 1 commit intomainfrom
fix/installer-tag

Conversation

@lroolle
Copy link
Copy Markdown
Member

@lroolle lroolle commented Mar 12, 2026

Summary

  • let install.sh combine DEVA_DOCKER_IMAGE with DEVA_DOCKER_TAG
  • stop CI smoke from exporting a doubly-tagged image name
  • keep local custom smoke images usable without registry pulls

Verification

  • shellcheck ./install.sh
  • DEVA_DOCKER_IMAGE=deva-smoke DEVA_DOCKER_TAG=ci bash ./install.sh
  • installed deva.sh claude -Q -- --version returned 2.1.72 (Claude Code)

Problem

main CI failed because the workflow built deva-smoke:ci but deva still looked for deva-smoke:ci:latest

- let install.sh combine DEVA_DOCKER_IMAGE with DEVA_DOCKER_TAG
- stop the smoke workflow from handing deva a double-tagged image name
- keep custom local smoke images usable without registry pulls
Copilot AI review requested due to automatic review settings March 12, 2026 07:51
@claude-yolo
Copy link
Copy Markdown
Contributor

claude-yolo bot commented Mar 12, 2026

Claude finished @lroolle's task in 2m 12s —— View job


Security Audit Complete ✅

Performed paranoid security audit of PR #216 focusing on container boundaries, auth flows, shell injection, and privilege escalation.

Todo List

  • Read and analyze install.sh changes for shell injection vectors
  • Review CI workflow changes for privilege/auth implications
  • Examine Docker image tagging logic for container boundary issues
  • Check for privilege escalation risks
  • Document blocking security issues
  • Identify required security tests before merge

Security Assessment: APPROVE WITH MONITORING

No Blocking Security Issues Found

Shell Injection: SECURE

  • printf '%s' format specifiers prevent format string injection
  • Docker commands properly quote variables: "$DOCKER_IMAGE"
  • Tested with malicious inputs - injection not possible

Command Injection: SECURE

  • Shell metacharacters passed as literal image name parts
  • Docker rejects invalid names gracefully
  • Proper variable quoting throughout

Logic Vulnerabilities: SECURE

  • image_ref() function handles edge cases correctly:
    • SHA references: repo@sha256:...
    • Pre-tagged images: repo:existing
    • Registry ports: localhost:5000/repo
    • Multiple colons handled properly ✓

⚠️ Existing Risk Vectors (Not Introduced by This PR)

Registry Confusion: INHERITED RISK

  • Users can specify arbitrary registries via DEVA_DOCKER_IMAGE
  • Examples: evil.com/malicious, localhost:5000/backdoor
  • This is by design for flexibility but creates attack surface
  • Recommendation: Document registry security in user docs

Docker Privilege: INHERITED RISK

  • Script runs docker pull with user privileges
  • Users with Docker access can already pull arbitrary images
  • Standard Docker security model applies

🔒 Required Security Tests Before Merge

  1. Shellcheck validation: shellcheck ./install.sh
  2. Integration test: Verify CI smoke test passes
  3. Manual verification: Test custom image handling
    DEVA_DOCKER_IMAGE=deva-smoke DEVA_DOCKER_TAG=ci bash ./install.sh

📋 Code Quality Notes

  • Clean separation of image name and tag logic
  • Proper handling of SHA references and existing tags
  • Maintains backward compatibility
  • Fixes CI double-tagging bug correctly

Recommendation: MERGE

This PR fixes a legitimate bug without introducing new security vulnerabilities. The changes are well-contained and properly handle Docker image reference construction.

@lroolle lroolle merged commit 6954c47 into main Mar 12, 2026
7 checks passed
@lroolle lroolle deleted the fix/installer-tag branch March 12, 2026 07:51
Copy link
Copy Markdown

Copilot AI left a comment

Choose a reason for hiding this comment

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

Pull request overview

This PR fixes Docker image reference construction for the installer and CI smoke test by separating repository and tag handling, preventing doubly-tagged image names and keeping locally-built smoke images usable.

Changes:

  • Added an image_ref helper in install.sh to combine image repo + tag (or preserve pre-tagged/digest refs) and updated default/fallback image resolution.
  • Updated CI smoke workflow to export DEVA_DOCKER_IMAGE and DEVA_DOCKER_TAG separately instead of passing a pre-tagged image string.

Reviewed changes

Copilot reviewed 2 out of 2 changed files in this pull request and generated 2 comments.

File Description
install.sh Introduces image_ref and updates how DOCKER_IMAGE / DOCKER_IMAGE_FALLBACK are computed from env vars.
.github/workflows/ci.yml Adjusts smoke job env vars to avoid passing a pre-tagged image name to the installer.

💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.

You can also share your feedback on Copilot code review. Take the survey.

Comment on lines +27 to +35
if [ -n "$tag" ]; then
printf '%s:%s' "$repo" "$tag"
return
fi

if [[ "$tail" == *:* ]]; then
printf '%s' "$repo"
return
fi
Copy link

Copilot AI Mar 12, 2026

Choose a reason for hiding this comment

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

image_ref appends :$tag before checking whether repo already contains a tag (via tail containing :). If DEVA_DOCKER_IMAGE is already tagged (e.g. deva-smoke:ci) and DEVA_DOCKER_TAG is also set, this will produce an invalid ref like deva-smoke:ci:ci. Consider detecting an existing tag first (or stripping it) when a separate tag is provided, and define clear precedence between an inline tag and DEVA_DOCKER_TAG to avoid generating invalid image refs.

Copilot uses AI. Check for mistakes.
Comment on lines +40 to +44
if [ -n "${DEVA_DOCKER_IMAGE+x}" ]; then
DOCKER_IMAGE="$(image_ref "$DEVA_DOCKER_IMAGE" "${DEVA_DOCKER_TAG:-}" "latest")"
else
DOCKER_IMAGE="$(image_ref "ghcr.io/thevibeworks/deva" "${DEVA_DOCKER_TAG:-}" "latest")"
fi
Copy link

Copilot AI Mar 12, 2026

Choose a reason for hiding this comment

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

The DEVA_DOCKER_IMAGE presence check uses ${DEVA_DOCKER_IMAGE+x}, so an explicitly-empty DEVA_DOCKER_IMAGE will be treated as set and passed to image_ref, resulting in an invalid image like :latest. Previously, an empty value would fall back to the default image via ${VAR:-default}. Consider treating empty DEVA_DOCKER_IMAGE the same as unset (e.g., check -n "${DEVA_DOCKER_IMAGE:-}"), or validate and error with a clear message.

Copilot uses AI. Check for mistakes.
Copy link
Copy Markdown

@chatgpt-codex-connector chatgpt-codex-connector bot left a comment

Choose a reason for hiding this comment

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

💡 Codex Review

Here are some automated review suggestions for this pull request.

Reviewed commit: 22f1ff7348

ℹ️ About Codex in GitHub

Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you

  • Open a pull request for review
  • Mark a draft as ready
  • Comment "@codex review".

If Codex has suggestions, it will comment; otherwise it will react with 👍.

Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".

Comment on lines +40 to +41
if [ -n "${DEVA_DOCKER_IMAGE+x}" ]; then
DOCKER_IMAGE="$(image_ref "$DEVA_DOCKER_IMAGE" "${DEVA_DOCKER_TAG:-}" "latest")"
Copy link
Copy Markdown

Choose a reason for hiding this comment

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

P2 Badge Treat empty DEVA_DOCKER_IMAGE as unset

If DEVA_DOCKER_IMAGE is present but empty (a common outcome of templated .env files), this branch still treats it as configured and calls image_ref with an empty repo, which produces :latest; the subsequent docker image inspect/docker pull then fails with an invalid image reference. Prior to this commit, ${DEVA_DOCKER_IMAGE:-...} fell back to the default image for empty values, so this is a regression in installer behavior for blank env configuration.

Useful? React with 👍 / 👎.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants