fix: centralize version tag logic across workflows - #4550
Conversation
- Creates scripts/install-ubuntu.sh for automated fresh Ubuntu server setup - Installs Apache, MariaDB, PHP 8.2 with required extensions - Downloads and configures OSPOS from GitHub - Sets up Apache virtual host with proper permissions - Generates secure random database password - Supports environment variables for customization - Updates INSTALL.md with curl pipe to bash instructions This provides an alternative to cloud-specific instructions and allows users to quickly set up OSPOS on any fresh Ubuntu server.
- Preferred install URL: https://opensourcepos.org/install - Falls back to direct GitHub URL if redirect unavailable - More professional and easier to remember
- Keep DigitalOcean referral link ($100 credit) - Simplify instructions to 3 steps: create droplet, SSH, run installer - Move one-line installation section into Cloud Install - Add security reminder to change password and configure SSL - Retain link to wiki for manual installation options
- Fetches latest release version from GitHub API - Downloads pre-built release zip instead of cloning repo - Renamed OSPOS_BRANCH to OSPOS_VERSION for clarity - Supports installing specific version via OSPOS_VERSION - Removed need for composer install (release is pre-built) - More stable for production deployments
- Adds Let's Encrypt support for production (with auto-renewal via certbot.timer) - Falls back to self-signed certificate for development/testing - New SSL_EMAIL environment variable enables production SSL - HTTPS redirect automatically configured for all sites - Updates INSTALL.md with SSL documentation and examples Production usage: SSL_EMAIL=admin@example.com APACHE_SERVER_NAME=pos.example.com Development usage (self-signed cert): APACHE_SERVER_NAME=localhost (default)
- Prompts user for SSL preferences during installation - Asks for domain name and email interactively - Falls back to environment variables for non-interactive mode - Shows SSL status in final output (Let's Encrypt / self-signed / none) - Updates INSTALL.md with interactive/non-interactive examples Interactive mode (recommended): curl -sSL https://opensourcepos.org/install | sudo bash # Prompts for SSL, domain, and email Non-interactive mode: curl -sSL https://opensourcepos.org/install | \ APACHE_SERVER_NAME=pos.example.com \ SSL_EMAIL=admin@example.com \ sudo -E bash
- Tests install script on Ubuntu 22.04 runner - Verifies Apache, MariaDB, and OSPOS services - Matrix tests default and custom DB_PASS scenarios - Uploads install logs as artifacts
The install script was failing on Ubuntu 22.04 because PHP 8.2 is not available in default repositories. This fix checks if the requested PHP version is available, and if not, adds the ondrej/php PPA which provides all supported PHP versions.
Releases use opensourcepos.VERSION.HASH.zip naming format, not opensourcepos-VERSION.zip. This fix fetches the actual asset URL from the GitHub API and extracts the correct directory name.
Let the install script use the latest release by default
The release zip files extract at root level without a subdirectory
Release zip contains .env directly, not .env.example. The sed commands to update database credentials were being skipped because the file check looked for .env.example first, which doesn't exist in published releases.
The .env file from release uses quoted values like 'localhost' but sed patterns were looking for unquoted values, causing database credentials to not be updated.
The release .env has an empty encryption key which causes HTTP 500 on startup. Generate a random key if the value is empty.
Hidden files like .env were not being copied because shell glob * doesn't match hidden files. Use 'ospos-temp/.' to copy all files.
The / delimiter was breaking when passwords contain special chars like !
- Remove duplicate php-gd package - Disable directory listing (Options -Indexes) - Propagate MYSQL_ROOT_PASS to all mysql commands - Fix allowedHostnames sed pattern to match .env.example format
- Add SQL injection validation for DB_NAME, DB_USER, DB_HOST - Honor OSPOS_DIR during extraction (was hardcoded to /var/www) - Fix SSL_EMAIL alone enabling Let's Encrypt (use APACHE_SERVER_NAME as domain) - Quote OSPOS_DIR variables to prevent word splitting
Base64 passwords can contain /, +, = which break sed with | delimiter. Also escape & and \ characters for safety.
Previously, version tags were generated inconsistently:
- build-release.yml: {VERSION}-{BRANCH}-{SHA-6}
- deploy-pr.yml: pr-{PR_NUMBER}-{SHA-7}
Additionally, deploy-pr.yml was broken because Docker images weren't
built for PR events, causing deployment failures.
Changes:
- Create shared .github/scripts/get-version.sh for version tag generation
- Standardize SHA length to 7 characters across all workflows
- Enable Docker builds for pull_request events in build-release.yml
- Update deploy-pr.yml to use shared script
Tag formats are now:
- PRs: pr-{NUMBER}-{SHA-7}
- Master push: {VERSION} (+ latest tag)
- Feature branches: {VERSION}-{BRANCH}-{SHA-7}
📝 WalkthroughWalkthroughAdds a reusable get-version.sh script used by build and deploy workflows for consistent image tags; adds scripts/install-ubuntu.sh installer, expands INSTALL.md with cloud install instructions, and adds a GitHub Actions workflow that runs and validates the installer in matrix scenarios. ChangesVersion tag generation refactoring
OSPOS Ubuntu installation automation
Estimated code review effort🎯 4 (Complex) | ⏱️ ~60 minutes Possibly related issues
Poem
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
There was a problem hiding this comment.
Actionable comments posted: 9
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (2)
.github/workflows/build-release.yml (2)
21-25:⚠️ Potential issue | 🟠 Major | ⚡ Quick winExpose
branchfrom the build job outputs.
Determine Docker tagsrelies onneeds.build.outputs.branch, butbranchis not exported frombuild.outputs. This makes themastercheck ineffective and can skip publishinglatest.Suggested fix
outputs: version: ${{ steps.version.outputs.version }} version-tag: ${{ steps.version.outputs.version-tag }} short-sha: ${{ steps.version.outputs.short-sha }} + branch: ${{ steps.version.outputs.branch }}Also applies to: 157-158
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In @.github/workflows/build-release.yml around lines 21 - 25, The build job currently exports version, version-tag, and short-sha but not branch, so downstream jobs using needs.build.outputs.branch fail; update the build job's outputs block to include branch: ${{ steps.version.outputs.branch }} (and mirror the same addition where the outputs are defined again near the later occurrence referenced) so that Determine Docker tags can read needs.build.outputs.branch correctly.
190-197:⚠️ Potential issue | 🟠 Major | ⚡ Quick winUse the same 7-char SHA in release artifact naming.
This step recomputes SHA with
--short=6, but artifacts are created with the 7-char SHA from the build step. That can make the release upload path point to a non-existent file.Suggested fix
- name: Get version info id: version run: | VERSION="${{ needs.build.outputs.version }}" - SHORT_SHA=$(git rev-parse --short=6 HEAD) + SHORT_SHA="${{ needs.build.outputs.short-sha }}" echo "version=$VERSION" >> $GITHUB_OUTPUT echo "short-sha=$SHORT_SHA" >> $GITHUB_OUTPUT🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In @.github/workflows/build-release.yml around lines 190 - 197, The workflow step "Get version info" recomputes the short SHA as SHORT_SHA=$(git rev-parse --short=6 HEAD) which yields a 6-char SHA while artifacts use the 7-char SHA from the build; update this step (id: version / variables VERSION and SHORT_SHA) to generate the same 7-character commit id (use --short=7) so the release artifact naming and upload paths match the build output.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In @.github/workflows/build-release.yml:
- Around line 130-131: The Docker publish step is currently gated by "if:
github.event_name == 'push' || github.event_name == 'pull_request'", which
allows forked PRs (without registry secrets) to run and fail; update the
condition for the Docker job/steps (the lines using that if) to only allow
pull_request runs from the same repository by changing it to something like: if:
github.event_name == 'push' || (github.event_name == 'pull_request' &&
github.event.pull_request.head.repo.full_name == github.repository). Apply this
same guarded condition to the other Docker publish-related block noted in the
comment (the later block around lines 145-150).
In @.github/workflows/install-script-test.yml:
- Around line 66-82: The login-page/content fetch doesn't follow redirects, so
when HTTP_CODE is 302 the later content checks (the curl calls that print page
HTML) will only show the redirect response instead of the final page; update the
curl invocations that fetch the site body to follow redirects by adding the -L
flag (e.g., replace curl -s http://localhost/ | head -50 with curl -s -L
http://localhost/ | head -50) and similarly add -L to any other curl body
fetches in the same block that inspect the page after HTTP_CODE is set (the
commands around HTTP_CODE and the subsequent HTML dumps).
- Around line 43-49: The GitHub Actions step "Run install script" currently
pipes the installer into tee which masks the install script's exit status;
update the step so the installer’s real exit code is preserved by enabling
pipefail before running scripts/install-ubuntu.sh (e.g., set -o pipefail or run
bash with -o pipefail) so the pipeline fails when scripts/install-ubuntu.sh
fails, and continue to capture output to install-output.log; ensure the change
is applied in the step that runs sudo -E bash scripts/install-ubuntu.sh and that
any sudo invocation still runs with pipefail enabled.
In `@INSTALL.md`:
- Line 113: Replace the broken short URL used in the main install command (the
line containing "curl -sSL https://opensourcepos.org/install") with the working
GitHub raw installer URL referenced later in the file (the alternative raw URL
shown at line 169); update that curl command to use the raw GitHub URL and
verify it returns the installer script (e.g., via a quick HEAD or GET check) so
the primary installation instruction points to a valid, non-404 endpoint.
In `@scripts/install-ubuntu.sh`:
- Around line 247-254: The script sets APACHE_SERVER_NAME to "localhost" when
empty which can conflict with a provided SSL_DOMAIN and causes writing
"localhost" into app.allowedHostnames in the .env output; change the
initialization so that if APACHE_SERVER_NAME is empty and SSL_DOMAIN is set,
APACHE_SERVER_NAME="$SSL_DOMAIN", otherwise default to "localhost". Also update
the .env writing logic (the block that writes app.allowedHostnames / FINAL_URL)
to prefer SSL_DOMAIN when APACHE_SERVER_NAME equals "localhost" but SSL_DOMAIN
is set, ensuring app.allowedHostnames and FINAL_URL use the public hostname
(reference variables: APACHE_SERVER_NAME, SSL_DOMAIN, FINAL_URL and the
app.allowedHostnames .env write).
- Around line 36-50: The DB validation currently allows characters that break
unquoted SQL identifiers and unescaped replacements; tighten validate_db_vars by
restricting DB_NAME/DB_USER/DB_HOST to safe identifier characters (e.g., remove
dot and hyphen so only [A-Za-z0-9_]) and/or require using quoted identifiers
(backticks) when DB_NAME is used as an identifier in SQL (functions/usages
referencing DB_NAME at creation and GRANT statements), and add explicit
validation/escaping for DB_PASS and MYSQL_ROOT_PASS to reject or properly escape
single quotes and the sed delimiter (pipe) before injecting into single-quoted
SQL or performing sed replacements; alternatively use a safer replacement method
(change sed delimiter to a character guaranteed not to appear or use a
quoting/escaping routine) and ensure .env writes use escaped/quoted values so
values like pa'ss|word cannot break SQL or corrupt .env.
- Around line 151-167: The current ASSET_URL assignment grabs the first
browser_download_url and may pick a non-zip asset; update the ASSET_URL
resolution (the line that sets ASSET_URL, which currently pipes curl into
grep/head/sed) to explicitly select the release asset whose URL or name ends
with ".zip" (or otherwise identify the zip asset) — e.g. filter the GitHub
release JSON for assets[].browser_download_url where the filename ends in .zip
(using jq or a more specific grep/sed) so curl -sSL "$ASSET_URL" -o ospos.zip
always downloads the zip that unzip -q ospos.zip -d ospos-temp expects. Ensure
the failure branch still triggers if no zip asset is found.
- Around line 114-120: When MYSQL_ROOT_PASS is empty, do not run the ALTER USER
statements that switch root off unix_socket; preserve unix_socket auth and use
sudo to run MariaDB commands instead of plain mysql. Update the conditional
around MYSQL_ROOT_PASS so the branch for empty value skips the ALTER USER
'root'@'localhost' IDENTIFIED BY ''/FLUSH PRIVILEGES calls and ensure subsequent
provisioning commands invoked via the mysql CLI use sudo mysql (e.g., replace
plain mysql -e invocations with sudo mysql -e when MYSQL_ROOT_PASS is unset);
when MYSQL_ROOT_PASS is set, keep the existing ALTER USER 'root'@'localhost'
IDENTIFIED BY '${MYSQL_ROOT_PASS}'; behavior.
- Around line 84-90: The current conditional using apt-cache policy ... | grep
-q "Candidate:" matches both real candidates and "Candidate: (none)"; change the
check to explicitly detect the "(none)" candidate and add the PPA only when the
candidate is none. Concretely, update the if that inspects apt-cache policy for
php${PHP_VERSION} to grep for "Candidate: (none)" (using the PHP_VERSION
variable and the existing apt-cache policy invocation) and run the
add-apt-repository / apt-get update block when that grep succeeds so the
ondrej/php PPA is added only when the package candidate is missing.
---
Outside diff comments:
In @.github/workflows/build-release.yml:
- Around line 21-25: The build job currently exports version, version-tag, and
short-sha but not branch, so downstream jobs using needs.build.outputs.branch
fail; update the build job's outputs block to include branch: ${{
steps.version.outputs.branch }} (and mirror the same addition where the outputs
are defined again near the later occurrence referenced) so that Determine Docker
tags can read needs.build.outputs.branch correctly.
- Around line 190-197: The workflow step "Get version info" recomputes the short
SHA as SHORT_SHA=$(git rev-parse --short=6 HEAD) which yields a 6-char SHA while
artifacts use the 7-char SHA from the build; update this step (id: version /
variables VERSION and SHORT_SHA) to generate the same 7-character commit id (use
--short=7) so the release artifact naming and upload paths match the build
output.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 06688b74-acf6-43ed-b0c8-36541491478d
📒 Files selected for processing (6)
.github/scripts/get-version.sh.github/workflows/build-release.yml.github/workflows/deploy-pr.yml.github/workflows/install-script-test.ymlINSTALL.mdscripts/install-ubuntu.sh
| - name: Verify Apache HTTP response | ||
| run: | | ||
| echo "Testing HTTP response on port 80..." | ||
| HTTP_CODE=$(curl -s -o /dev/null -w "%{http_code}" http://localhost/) | ||
| echo "HTTP Response Code: $HTTP_CODE" | ||
| if [ "$HTTP_CODE" = "200" ] || [ "$HTTP_CODE" = "302" ]; then | ||
| echo "Apache is responding correctly" | ||
| elif [ "$HTTP_CODE" = "500" ]; then | ||
| echo "HTTP 500 - Application error. Checking .env configuration..." | ||
| sudo cat /var/www/ospos/.env 2>/dev/null | grep -E "database\.default\.(hostname|database|username|password)|encryption\.key|CI_ENVIRONMENT" | head -10 | ||
| sudo cat /var/www/ospos/writable/logs/*.log 2>/dev/null | tail -20 || true | ||
| curl -s http://localhost/ | head -50 | ||
| exit 1 | ||
| else | ||
| echo "Unexpected HTTP code: $HTTP_CODE" | ||
| exit 1 | ||
| fi |
There was a problem hiding this comment.
Follow redirects in the login-page check.
Lines 71-72 explicitly treat 302 as a healthy response, but Lines 87-89 fetch / without -L. If OSPOS redirects / to the login route, this step will inspect the redirect response instead of the page and fail a good install.
Suggested fix
- curl -s http://localhost/ | grep -qi "login\|password\|username" && echo "Login page content found" || {
+ curl -sL http://localhost/ | grep -qi "login\|password\|username" && echo "Login page content found" || {
echo "Login page verification failed"
- curl -s http://localhost/ | head -50
+ curl -sL http://localhost/ | head -50
exit 1
}Also applies to: 84-90
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In @.github/workflows/install-script-test.yml around lines 66 - 82, The
login-page/content fetch doesn't follow redirects, so when HTTP_CODE is 302 the
later content checks (the curl calls that print page HTML) will only show the
redirect response instead of the final page; update the curl invocations that
fetch the site body to follow redirects by adding the -L flag (e.g., replace
curl -s http://localhost/ | head -50 with curl -s -L http://localhost/ | head
-50) and similarly add -L to any other curl body fetches in the same block that
inspect the page after HTTP_CODE is set (the commands around HTTP_CODE and the
subsequent HTML dumps).
| 2. SSH into your server: `ssh root@<your-droplet-ip>` | ||
| 3. Run the one-line installer: | ||
| ```bash | ||
| curl -sSL https://opensourcepos.org/install | sudo bash |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Description: Verify the short URL redirects to the expected GitHub script
# Check if the short URL is accessible and returns the script content
HTTP_CODE=$(curl -sSL -o /dev/null -w "%{http_code}" https://opensourcepos.org/install)
echo "HTTP Status: $HTTP_CODE"
# Verify it redirects to GitHub
REDIRECT_URL=$(curl -sSL -w "%{url_effective}" -o /dev/null https://opensourcepos.org/install)
echo "Redirect URL: $REDIRECT_URL"
# Check if content looks like a bash script
FIRST_LINE=$(curl -sSL https://opensourcepos.org/install | head -n1)
echo "First line: $FIRST_LINE"
if [[ "$FIRST_LINE" == "#!/bin/bash"* ]]; then
echo "✓ Short URL serves a bash script"
else
echo "✗ Short URL does not serve a bash script"
fiRepository: opensourcepos/opensourcepos
Length of output: 190
Fix the broken short URL—the primary install command will fail.
The short URL https://opensourcepos.org/install returns a 404 error and does not serve the installer script. Users following the main installation instruction at line 113 will be blocked. Either restore the short URL or update the documentation to use the alternative GitHub raw URL from line 169 as the primary method.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@INSTALL.md` at line 113, Replace the broken short URL used in the main
install command (the line containing "curl -sSL
https://opensourcepos.org/install") with the working GitHub raw installer URL
referenced later in the file (the alternative raw URL shown at line 169); update
that curl command to use the raw GitHub URL and verify it returns the installer
script (e.g., via a quick HEAD or GET check) so the primary installation
instruction points to a valid, non-404 endpoint.
| # Validate database variables contain only safe characters (alphanumeric, underscore, hyphen, dot) | ||
| validate_db_vars() { | ||
| local var_name="$1" | ||
| local var_value="$2" | ||
| local pattern='^[a-zA-Z0-9_\-\.]+$' | ||
| if [[ ! "$var_value" =~ $pattern ]]; then | ||
| echo -e "${COLOR_RED}Error: ${var_name} contains invalid characters. Only alphanumeric, underscore, hyphen, and dot are allowed.${COLOR_RESET}" | ||
| exit 1 | ||
| fi | ||
| } | ||
|
|
||
| # Validate critical database variables | ||
| validate_db_vars "DB_NAME" "$DB_NAME" | ||
| validate_db_vars "DB_USER" "$DB_USER" | ||
| validate_db_vars "DB_HOST" "$DB_HOST" |
There was a problem hiding this comment.
The current DB var handling still accepts values the SQL and .env paths can't safely consume.
DB_NAME allows . and - even though Lines 125-127 and 132-134 use it as an unquoted identifier, and DB_PASS/MYSQL_ROOT_PASS are injected verbatim into single-quoted SQL plus a sed replacement that still doesn't escape the | delimiter. Inputs like my-db or pa'ss|word will break the install or corrupt .env.
Also applies to: 119-137, 189-196
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@scripts/install-ubuntu.sh` around lines 36 - 50, The DB validation currently
allows characters that break unquoted SQL identifiers and unescaped
replacements; tighten validate_db_vars by restricting DB_NAME/DB_USER/DB_HOST to
safe identifier characters (e.g., remove dot and hyphen so only [A-Za-z0-9_])
and/or require using quoted identifiers (backticks) when DB_NAME is used as an
identifier in SQL (functions/usages referencing DB_NAME at creation and GRANT
statements), and add explicit validation/escaping for DB_PASS and
MYSQL_ROOT_PASS to reject or properly escape single quotes and the sed delimiter
(pipe) before injecting into single-quoted SQL or performing sed replacements;
alternatively use a safer replacement method (change sed delimiter to a
character guaranteed not to appear or use a quoting/escaping routine) and ensure
.env writes use escaped/quoted values so values like pa'ss|word cannot break SQL
or corrupt .env.
| # Add PHP repository for newer PHP versions if not available in default repos | ||
| if ! apt-cache policy php${PHP_VERSION} 2>/dev/null | grep -q "Candidate:"; then | ||
| echo -e "${COLOR_YELLOW}PHP ${PHP_VERSION} not in default repos, adding ondrej/php PPA...${COLOR_RESET}" | ||
| apt-get install -y -qq software-properties-common | ||
| add-apt-repository -y ppa:ondrej/php | ||
| apt-get update -qq | ||
| fi |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
# First, let's read the actual file to confirm the current implementation
cat -n scripts/install-ubuntu.sh | sed -n '84,90p'Repository: opensourcepos/opensourcepos
Length of output: 509
🌐 Web query:
apt-cache policy output format for unavailable packages
💡 Result:
For a package that is unavailable to APT, apt-cache policy typically shows Installed: (none) and Candidate: (none), and in the Version table you’ll usually see no available versions. apt-cache “policy” is designed to show pinning priorities and the candidate selection for the named package [1].
Citations:
🌐 Web query:
apt-cache show command behavior for missing packages
💡 Result:
When you run apt-cache against the APT package cache, the “missing packages” behavior is determined by which apt-cache subcommand you use and what metadata is available in that cache. 1) apt-cache show (missing package name) - apt-cache show only queries and prints records “for the named packages” [1][2]. It does not state an alternative/placeholder output when the named package is absent; in practice it results in no package record being printed (i.e., essentially empty/“nothing found”) because there is no matching package entry to display. This matches the manpage’s description that it “displays the package records for the named packages” [1][2]. 2) apt-cache unmet and “Missing packages” counts (unmet dependencies) - apt-cache unmet reports “a summary of all unmet dependencies in the package cache” [1][2]. - Separately, for the dependency-related output, apt-cache statistics-style output defines “Missing” as “the number of package names that were referenced in a dependency but were not provided by any package” [3][1][2]. This is about dependency relations referencing names that no package provides in the current cache, not about whether the user requested a particular package name with apt-cache show. - That means you can see “missing packages”/unmet entries even if the packages themselves are not installed and even if some referenced dependency names no longer exist in the distribution metadata that’s currently loaded into your APT cache (or your sources are incomplete/out-of-date). The apt-cache docs explicitly note this interpretation: “Missing packages may be evidence if a full distribution is not accessed, or if a package (real or virtual) has been dropped from the distribution” [3][1][2]. 3) Why this happens after restoring old configs / fresh install - apt-cache uses the metadata from the local APT package lists/cache; it “does not manipulate the state of the system” and “the metadata is acquired and updated” (via apt-get update, etc.), but apt-cache can work “independently of the availability of the configured sources” and can be “outdated if the last update is too long ago” [4]. - Therefore, after changing/restoring sources or configs, your local package lists may still contain (or lack) dependency relationships, leading to unmet/missing dependency names in the apt-cache view [4][1]. 4) How to reduce confusion / narrow scope - Use apt-cache commands that match your intent: a) To inspect a particular package’s metadata, use apt-cache show (it only prints records that exist in the cache for that exact name) [1][2]. b) To inspect missing/unmet dependency relationships across the cache, use apt-cache unmet (it summarizes unmet dependencies across the cache, not just installed packages) [1][2]. - If you see huge amounts of unmet output, there is a manpage option -i/--important to narrow what unmet prints: “Print only important dependencies … for use with unmet and depends” [5]. Practical takeaway - apt-cache show: if the named package does not exist in the currently loaded cache, there is no package record to print (so you effectively get no output/record for that name) [1][2]. - apt-cache unmet / dependency “Missing” counts: these refer to dependency targets referenced by other packages but not provided by any package in the cache (often due to stale/out-of-date package lists, incomplete sources, or packages dropped from the distribution) [3][1][2].
Citations:
- 1: https://manpages.ubuntu.com/manpages/trusty/en/man8/apt-cache.8.html
- 2: https://manpages.ubuntu.com/manpages/xenial/man8/apt-cache.8.html
- 3: https://manpages.debian.org/bullseye/apt/apt-cache.8.en.html
- 4: https://man.cx/apt-cache(8)
- 5: https://askubuntu.com/questions/716876/apt-cache-unmet-shows-non-existent-packages
🏁 Script executed:
# Verify the actual behavior difference between apt-cache policy and apt-cache show
# by checking Ubuntu/Debian documentation and common usage patterns
# Let's also check if there are any other PHP version checks in the codebase
rg -n "php" scripts/install-ubuntu.sh | head -20Repository: opensourcepos/opensourcepos
Length of output: 719
🌐 Web query:
apt-cache show exit code behavior missing package ubuntu
💡 Result:
apt-cache show does not reliably use a non-zero exit code to indicate “missing package” (no package record found). For a non-existent package name, the command may print errors but still return exit status 0 in at least some cases/configurations. Evidence: In an Ubuntu bug report, running apt-cache show gedit > /dev/null; echo $? returns 0 even though LC_ALL=C apt-cache show gedita > /dev/null; echo $? returns an error message and a different exit status in that scenario [1]. Another authoritative reference states only that apt-cache returns zero on normal operation and decimal 100 on error (but “missing package” appears to be treated as not an error in some implementations/uses) [2]. What to do instead (recommended): 1) Use dpkg-query for installed packages. It provides configurable, machine-parseable status/formatting and has well-defined semantics. For example, to get an installed version you can use: dpkg-query --showformat='${Version}' --show (This works for installed packages) [3]. 2) If you must use apt-cache, don’t rely on its exit code for “not found”; parse its output (e.g., look for “Unable to locate package …”) or check availability first (e.g., apt-cache show <pkg> then confirm that a Version: line exists). The apt-cache man page notes show “displays the package records” for named packages and that apt-cache “does provide operations to search and generate interesting output from the package metadata” based on cached lists [4]. So the practical answer to your query is: for “missing package ubuntu” (i.e., query a package name that does not exist in the cache), don’t depend on apt-cache show exit codes; use output parsing or switch to dpkg-query when you specifically need installed versions [3].
Citations:
- 1: https://bugs.launchpad.net/bugs/1037521
- 2: https://man.cx/apt-cache(8)
- 3: https://askubuntu.com/questions/1476913/apt-cache-show-package-field-show-single-field-of-a-package
- 4: https://manpages.ubuntu.com/manpages/focal/man8/apt-cache.8.html
🏁 Script executed:
# Verify the exact apt-cache policy output format to design the best fix
# Let's check what a proper detection pattern should be
# Search for any documentation or comments about package detection in related scripts
rg -A5 -B5 "apt-cache" scripts/Repository: opensourcepos/opensourcepos
Length of output: 926
🌐 Web query:
apt-cache policy output format candidate version none ubuntu
💡 Result:
When you run the command apt-cache policy <package_name> on Ubuntu or Debian, the output provides information about the package's installation status, the candidate version for installation, and the available versions in your configured repositories [1][2][3]. The Candidate field specifies the version of the package that APT would select for installation if you were to run an install command [4]. If this field displays (none), it indicates that APT cannot find any available version of that package in your currently configured software sources [1][3]. This typically happens for one of the following reasons: 1. The package name is misspelled or does not exist in the repositories currently defined in your /etc/apt/sources.list or /etc/apt/sources.list.d/ directory. 2. You have not run apt update recently, meaning your local package cache is outdated and does not reflect the current contents of the repositories. 3. The repository containing the package is not enabled on your system. To resolve this, ensure the package name is correct, run sudo apt update to refresh your local cache, and verify that the necessary repositories are enabled [4][2].
Citations:
- 1: https://unix.stackexchange.com/questions/121413/understanding-the-output-of-apt-cache-policy
- 2: https://debian-handbook.info/browse/stable/sect.apt-cache.html
- 3: https://stackoverflow.com/questions/18885820/how-can-i-check-the-version-before-installing-a-package-using-apt-get
- 4: https://askubuntu.com/questions/1367352/multiple-outputs-for-apt-cache-policy-package-name
Fix package availability detection for PHP version check.
Line 85 checks for the presence of Candidate: in apt-cache policy output, which matches both available versions (Candidate: X.Y.Z) and unavailable packages (Candidate: (none)). This causes the condition to fail in both cases, preventing the PPA from being added when the PHP version is actually unavailable.
The suggested fix using apt-cache show with an exit code check is unreliable; apt-cache show does not consistently return non-zero for missing packages across different Ubuntu configurations. Instead, explicitly match a non-none candidate:
Suggested fix
-if ! apt-cache policy php${PHP_VERSION} 2>/dev/null | grep -q "Candidate:"; then
+if apt-cache policy php${PHP_VERSION} 2>/dev/null | grep -q "Candidate: (none)"; then
echo -e "${COLOR_YELLOW}PHP ${PHP_VERSION} not in default repos, adding ondrej/php PPA...${COLOR_RESET}"
apt-get install -y -qq software-properties-common
add-apt-repository -y ppa:ondrej/php
apt-get update -qq
fi🧰 Tools
🪛 Shellcheck (0.11.0)
[info] 85-85: Double quote to prevent globbing and word splitting.
(SC2086)
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@scripts/install-ubuntu.sh` around lines 84 - 90, The current conditional
using apt-cache policy ... | grep -q "Candidate:" matches both real candidates
and "Candidate: (none)"; change the check to explicitly detect the "(none)"
candidate and add the PPA only when the candidate is none. Concretely, update
the if that inspects apt-cache policy for php${PHP_VERSION} to grep for
"Candidate: (none)" (using the PHP_VERSION variable and the existing apt-cache
policy invocation) and run the add-apt-repository / apt-get update block when
that grep succeeds so the ondrej/php PPA is added only when the package
candidate is missing.
| if [ -z "$MYSQL_ROOT_PASS" ]; then | ||
| echo -e "${COLOR_BLUE}Securing MariaDB installation...${COLOR_RESET}" | ||
| mysql -e "ALTER USER 'root'@'localhost' IDENTIFIED BY '';" | ||
| mysql -e "FLUSH PRIVILEGES;" | ||
| else | ||
| mysql -e "ALTER USER 'root'@'localhost' IDENTIFIED BY '${MYSQL_ROOT_PASS}';" | ||
| fi |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
# First, let's examine the full script to understand the context
wc -l scripts/install-ubuntu.shRepository: opensourcepos/opensourcepos
Length of output: 101
🏁 Script executed:
# Read the relevant section of the install script with surrounding context
sed -n '100,130p' scripts/install-ubuntu.shRepository: opensourcepos/opensourcepos
Length of output: 1094
🏁 Script executed:
# Search for any references to MYSQL_ROOT_PASS setup in the script
rg "MYSQL_ROOT_PASS" scripts/install-ubuntu.sh -B 3 -A 3Repository: opensourcepos/opensourcepos
Length of output: 1334
🏁 Script executed:
# Check if there are other places where MariaDB/MySQL authentication is configured
rg -i "mysql|mariadb" scripts/install-ubuntu.sh | head -20Repository: opensourcepos/opensourcepos
Length of output: 859
🌐 Web query:
MariaDB default root authentication method Ubuntu unix_socket
💡 Result:
On Ubuntu (MariaDB packaged via Debian/Ubuntu repos), the MariaDB root account is configured to authenticate using the unix_socket authentication plugin for the 'root'@'localhost' user account by default.[1][2] MariaDB’s unix_socket plugin is installed by default and “is used by the 'root'@'localhost' user account by default.”[1][2] The authentication mechanism allows login without a MariaDB password via the local Unix socket (OS credentials), defined by MariaDB’s socket system variable.[1] Also note that MariaDB 10.4+ installation creates root@localhost with an OR between unix_socket and mysql_native_password, where mysql_native_password starts with an invalid password hash; password auth requires setting a real password with SET PASSWORD, while unix_socket allows local OS-root logins over the Unix socket.[3]
Citations:
- 1: https://mariadb.com/docs/server/reference/plugins/authentication-plugins/authentication-plugin-unix-socket.md
- 2: https://mariadb.com/docs/server/reference/plugins/authentication-plugins/authentication-plugin-unix-socket
- 3: https://mariadb.com/docs/server/security/user-account-management/authentication-from-mariadb-10-4
🌐 Web query:
MariaDB unix_socket authentication vs password authentication security Ubuntu
💡 Result:
In MariaDB on Ubuntu, the unix_socket and password-based (e.g., mysql_native_password) authentication methods offer different security models. The unix_socket plugin is the default for the root user in standard Ubuntu installations [1][2]. Unix_socket Authentication This method is passwordless and relies on the security of the operating system [1][2]. When a user connects via a local Unix socket, MariaDB uses the SO_PEERCRED socket option to retrieve the UID of the connecting process and maps it to the corresponding MariaDB user [3]. - Security Strengths: It eliminates the risk of password brute-force attacks and prevents password exposure through configuration files, backups, or accidental leakage [1][2]. Because it requires local access, it is inherently resistant to remote eavesdropping [4]. - Security Dependencies: Its security is entirely dependent on the security of the underlying Unix user accounts [1][2]. If an unauthorized user gains access to a specific OS account, they can connect to the database as that user without needing a separate database password [1][2]. - Limitations: It only works for local connections [5]. It cannot be used for remote network connections [5]. Password Authentication Traditional password authentication (e.g., mysql_native_password) requires the user to provide a password, which is then verified by the database [6]. - Security Strengths: It provides a layer of authentication independent of the OS user, which is necessary for remote connections [5]. - Security Risks: Passwords can be weak, reused, or exposed in configuration files, scripts, or backups [1][2]. If a password is compromised, an attacker may gain access regardless of their OS-level privileges [4]. Comparison and Best Practices - Default Configuration: Ubuntu's default MariaDB installation uses unix_socket for the root user to enhance security by ensuring that only the system root user (or users with sudo privileges) can access the database root account [1][2]. - Hybrid Approach: Since MariaDB 10.4, you can configure accounts to support multiple authentication methods using the OR keyword [7][4]. For example, you can allow unix_socket for local administrative tasks while requiring a strong password for remote or application-based connections [7]. - Recommendation: Use unix_socket for local administrative access to minimize password management risks [1][2]. For application users or remote access, use strong, unique passwords combined with appropriate network restrictions (e.g., binding to 127.0.0.1 or using firewalls) [8][9]. Avoid using the same account for both local socket and remote password access if possible, or ensure the password is highly secure [4].
Citations:
- 1: https://mariadb.com/docs/server/reference/plugins/authentication-plugins/authentication-plugin-unix-socket.md
- 2: https://mariadb.com/docs/server/reference/plugins/authentication-plugins/authentication-plugin-unix-socket
- 3: https://mariadb.com/docs/server/reference/plugins/authentication-plugins/pluggable-authentication-overview.md
- 4: https://stackoverflow.com/questions/66429827/mariadb-unix-socket-without-password-or-mysql-native-password-what-is-safer
- 5: https://www.tenable.com/audits/items/CIS_MariaDB_10.11_v1.0.0_L2_MariaDB_RDBMS_MySQLDB.audit:7ffaf455cf4e0ce35e0a388a941027ad
- 6: https://www.linuxbabe.com/mariadb/plugin-unix_socket-is-not-loaded-2
- 7: https://stackoverflow.com/questions/41846000/enable-password-and-unix-socket-authentication-for-mariadb-root-user
- 8: https://www.blogarama.com/technology-blogs/1455526-green-webpage-blog/76185222-install-secure-mysqlmariadb-almalinux-step-guide
- 9: https://dev.to/vultr/installing-the-lemp-stack-on-ubuntu-2604-3m2k
🌐 Web query:
Ubuntu MariaDB root@localhost default auth socket
💡 Result:
On Ubuntu, MariaDB installations use the unix_socket authentication plugin by default for the root@localhost user [1][2][3]. This security feature allows you to log in to the MariaDB root account without a password, provided you are logged into the operating system as the root user (typically by using sudo) [4][5][6]. How it works: The unix_socket plugin verifies your identity by checking the operating system user credentials of the process attempting to connect via the local Unix socket file [1][3]. Because the system root user already has full access to the server's data files and memory, requiring a separate database password for the root user is considered redundant and less secure than relying on OS-level authentication [6]. Accessing the database: To connect to MariaDB as root, you should use the following command: sudo mysql -u root If you attempt to connect using mysql -u root -p without sudo, or if you try to provide a password when one has not been set, you will likely receive an "Access denied" error [5][7]. Configuring password authentication: In modern MariaDB versions (10.4 and later), the root account is often configured to support both unix_socket and password-based authentication simultaneously [4][6]. The password method is disabled by default (set to an invalid hash) [3][6]. If you wish to enable password-based access (for example, to allow applications or remote tools to connect), you can set a password while retaining the ability to use sudo for passwordless local access [6]: ALTER USER 'root'@'localhost' IDENTIFIED VIA mysql_native_password USING PASSWORD('your_secure_password'); After running this, you can still use sudo mysql -u root to log in without a password, but you will also be able to log in using the password you just set [6]. Note that if you are connecting via TCP (e.g., 127.0.0.1) rather than the Unix socket, you must use the password [8].
Citations:
- 1: https://mariadb.com/docs/server/reference/plugins/authentication-plugins/authentication-plugin-unix-socket.md
- 2: https://mariadb.com/docs/server/reference/plugins/authentication-plugins/authentication-plugin-unix-socket
- 3: https://mariadb.com/docs/server/reference/plugins/authentication-plugins/pluggable-authentication-overview.md
- 4: https://www.simplified.guide/mysql-mariadb/root-access-without-sudo
- 5: https://stackoverflow.com/questions/28068155/access-denied-for-user-rootlocalhost-using-password-yes-after-new-instal
- 6: https://mariadb.org/authentication-in-mariadb-10-4
- 7: https://stackoverflow.com/questions/43379892/mariadb-cannot-login-as-root
- 8: https://stackoverflow.com/questions/41846000/enable-password-and-unix-socket-authentication-for-mariadb-root-user
Replace empty-password root authentication with sudo access to preserve unix_socket security.
On Ubuntu, MariaDB's default root@localhost authentication uses the unix_socket plugin—a passwordless mechanism that relies on OS credentials (accessed via sudo mysql). Lines 116–117 replace this secure socket-based auth with password authentication using an empty password, which weakens the installation. When MYSQL_ROOT_PASS is unset, the root account should retain unix_socket authentication; update subsequent database operations to use sudo mysql instead of bare mysql commands for provisioning.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@scripts/install-ubuntu.sh` around lines 114 - 120, When MYSQL_ROOT_PASS is
empty, do not run the ALTER USER statements that switch root off unix_socket;
preserve unix_socket auth and use sudo to run MariaDB commands instead of plain
mysql. Update the conditional around MYSQL_ROOT_PASS so the branch for empty
value skips the ALTER USER 'root'@'localhost' IDENTIFIED BY ''/FLUSH PRIVILEGES
calls and ensure subsequent provisioning commands invoked via the mysql CLI use
sudo mysql (e.g., replace plain mysql -e invocations with sudo mysql -e when
MYSQL_ROOT_PASS is unset); when MYSQL_ROOT_PASS is set, keep the existing ALTER
USER 'root'@'localhost' IDENTIFIED BY '${MYSQL_ROOT_PASS}'; behavior.
| echo -e "${COLOR_BLUE}Downloading OSPOS version ${OSPOS_VERSION}...${COLOR_RESET}" | ||
| ASSET_URL=$(curl -sS "https://api.github.com/repos/opensourcepos/opensourcepos/releases/tags/${OSPOS_VERSION}" | grep '"browser_download_url"' | head -1 | sed -E 's/.*"([^"]+)".*/\1/') | ||
|
|
||
| if [ -z "$ASSET_URL" ]; then | ||
| echo -e "${COLOR_RED}Failed to find release asset for ${OSPOS_VERSION}${COLOR_RESET}" | ||
| exit 1 | ||
| fi | ||
|
|
||
| curl -sSL "$ASSET_URL" -o ospos.zip | ||
|
|
||
| if [ ! -f ospos.zip ] || [ ! -s ospos.zip ]; then | ||
| echo -e "${COLOR_RED}Failed to download OSPOS release ${OSPOS_VERSION}${COLOR_RESET}" | ||
| rm -f ospos.zip | ||
| exit 1 | ||
| fi | ||
|
|
||
| unzip -q ospos.zip -d ospos-temp |
There was a problem hiding this comment.
Select the zip asset instead of the first release asset.
Line 152 grabs the first browser_download_url blindly. If a release has multiple uploaded assets, unzip can be pointed at the wrong file and the rest of the install fails for a healthy release.
Suggested fix
-ASSET_URL=$(curl -sS "https://api.github.com/repos/opensourcepos/opensourcepos/releases/tags/${OSPOS_VERSION}" | grep '"browser_download_url"' | head -1 | sed -E 's/.*"([^"]+)".*/\1/')
+ASSET_URL=$(
+ curl -sS "https://api.github.com/repos/opensourcepos/opensourcepos/releases/tags/${OSPOS_VERSION}" \
+ | grep '"browser_download_url"' \
+ | grep -E '\.zip"' \
+ | head -1 \
+ | sed -E 's/.*"([^"]+)".*/\1/'
+)📝 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.
| echo -e "${COLOR_BLUE}Downloading OSPOS version ${OSPOS_VERSION}...${COLOR_RESET}" | |
| ASSET_URL=$(curl -sS "https://api.github.com/repos/opensourcepos/opensourcepos/releases/tags/${OSPOS_VERSION}" | grep '"browser_download_url"' | head -1 | sed -E 's/.*"([^"]+)".*/\1/') | |
| if [ -z "$ASSET_URL" ]; then | |
| echo -e "${COLOR_RED}Failed to find release asset for ${OSPOS_VERSION}${COLOR_RESET}" | |
| exit 1 | |
| fi | |
| curl -sSL "$ASSET_URL" -o ospos.zip | |
| if [ ! -f ospos.zip ] || [ ! -s ospos.zip ]; then | |
| echo -e "${COLOR_RED}Failed to download OSPOS release ${OSPOS_VERSION}${COLOR_RESET}" | |
| rm -f ospos.zip | |
| exit 1 | |
| fi | |
| unzip -q ospos.zip -d ospos-temp | |
| echo -e "${COLOR_BLUE}Downloading OSPOS version ${OSPOS_VERSION}...${COLOR_RESET}" | |
| ASSET_URL=$( | |
| curl -sS "https://api.github.com/repos/opensourcepos/opensourcepos/releases/tags/${OSPOS_VERSION}" \ | |
| | grep '"browser_download_url"' \ | |
| | grep -E '\.zip"' \ | |
| | head -1 \ | |
| | sed -E 's/.*"([^"]+)".*/\1/' | |
| ) | |
| if [ -z "$ASSET_URL" ]; then | |
| echo -e "${COLOR_RED}Failed to find release asset for ${OSPOS_VERSION}${COLOR_RESET}" | |
| exit 1 | |
| fi | |
| curl -sSL "$ASSET_URL" -o ospos.zip | |
| if [ ! -f ospos.zip ] || [ ! -s ospos.zip ]; then | |
| echo -e "${COLOR_RED}Failed to download OSPOS release ${OSPOS_VERSION}${COLOR_RESET}" | |
| rm -f ospos.zip | |
| exit 1 | |
| fi | |
| unzip -q ospos.zip -d ospos-temp |
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@scripts/install-ubuntu.sh` around lines 151 - 167, The current ASSET_URL
assignment grabs the first browser_download_url and may pick a non-zip asset;
update the ASSET_URL resolution (the line that sets ASSET_URL, which currently
pipes curl into grep/head/sed) to explicitly select the release asset whose URL
or name ends with ".zip" (or otherwise identify the zip asset) — e.g. filter the
GitHub release JSON for assets[].browser_download_url where the filename ends in
.zip (using jq or a more specific grep/sed) so curl -sSL "$ASSET_URL" -o
ospos.zip always downloads the zip that unzip -q ospos.zip -d ospos-temp
expects. Ensure the failure branch still triggers if no zip asset is found.
| if [ -z "$APACHE_SERVER_NAME" ]; then | ||
| APACHE_SERVER_NAME="localhost" | ||
| fi | ||
|
|
||
| # If SSL_EMAIL is set without SSL_DOMAIN, use APACHE_SERVER_NAME | ||
| if [ -n "$SSL_EMAIL" ] && [ -z "$SSL_DOMAIN" ] && [ "$APACHE_SERVER_NAME" != "localhost" ]; then | ||
| SSL_DOMAIN="$APACHE_SERVER_NAME" | ||
| fi |
There was a problem hiding this comment.
Use the public hostname when writing app.allowedHostnames.
If the caller sets SSL_DOMAIN but leaves APACHE_SERVER_NAME unset, Line 248 forces localhost; the HTTPS vhost and FINAL_URL still use SSL_DOMAIN, but Line 353 writes localhost into .env. Domain-based installs will then reject the real host header.
Suggested fix
-# Set default server name if not provided
-if [ -z "$APACHE_SERVER_NAME" ]; then
- APACHE_SERVER_NAME="localhost"
-fi
-
-# If SSL_EMAIL is set without SSL_DOMAIN, use APACHE_SERVER_NAME
-if [ -n "$SSL_EMAIL" ] && [ -z "$SSL_DOMAIN" ] && [ "$APACHE_SERVER_NAME" != "localhost" ]; then
+# Prefer the public SSL host when present
+if [ -n "$SSL_DOMAIN" ] && [ -z "$APACHE_SERVER_NAME" ]; then
+ APACHE_SERVER_NAME="$SSL_DOMAIN"
+elif [ -z "$APACHE_SERVER_NAME" ]; then
+ APACHE_SERVER_NAME="localhost"
+fi
+
+# If SSL_EMAIL is set without SSL_DOMAIN, use APACHE_SERVER_NAME
+if [ -n "$SSL_EMAIL" ] && [ -z "$SSL_DOMAIN" ] && [ "$APACHE_SERVER_NAME" != "localhost" ]; then
SSL_DOMAIN="$APACHE_SERVER_NAME"
fiAlso applies to: 351-353
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@scripts/install-ubuntu.sh` around lines 247 - 254, The script sets
APACHE_SERVER_NAME to "localhost" when empty which can conflict with a provided
SSL_DOMAIN and causes writing "localhost" into app.allowedHostnames in the .env
output; change the initialization so that if APACHE_SERVER_NAME is empty and
SSL_DOMAIN is set, APACHE_SERVER_NAME="$SSL_DOMAIN", otherwise default to
"localhost". Also update the .env writing logic (the block that writes
app.allowedHostnames / FINAL_URL) to prefer SSL_DOMAIN when APACHE_SERVER_NAME
equals "localhost" but SSL_DOMAIN is set, ensuring app.allowedHostnames and
FINAL_URL use the public hostname (reference variables: APACHE_SERVER_NAME,
SSL_DOMAIN, FINAL_URL and the app.allowedHostnames .env write).
- Add missing 'branch' output to build job (fixes master detection) - Guard Docker publish against fork PRs (secrets unavailable) - Use consistent 7-char SHA from build outputs in release job - Add pipefail to install script test to preserve exit status
There was a problem hiding this comment.
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
.github/workflows/install-script-test.yml (1)
76-77:⚠️ Potential issue | 🟠 Major | ⚡ Quick winDo not print secrets from
.envinto CI logs.The failure path currently logs
database.default.passwordandencryption.key, which can leak sensitive values in Actions logs/artifacts.Suggested minimal hardening
- sudo cat /var/www/ospos/.env 2>/dev/null | grep -E "database\.default\.(hostname|database|username|password)|encryption\.key|CI_ENVIRONMENT" | head -10 + sudo cat /var/www/ospos/.env 2>/dev/null | grep -E "database\.default\.(hostname|database|username)|CI_ENVIRONMENT" | head -10 + sudo grep -q '^encryption\.key=' /var/www/ospos/.env 2>/dev/null && echo "encryption.key is set"🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In @.github/workflows/install-script-test.yml around lines 76 - 77, The CI step currently leaks secrets by grepping and printing `.env` entries that include `database.default.password` and `encryption.key`; update the command that reads `.env` (the line that currently greps for "database.default.(hostname|database|username|password)|encryption.key|CI_ENVIRONMENT") to avoid emitting secret values—either stop matching the password and encryption.key keys at all or pipe the matched lines through a redaction step so only keys (or keys with a fixed "[REDACTED]" value) are printed; ensure the change affects the command that also tails logs so CI output no longer contains raw secret values.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Outside diff comments:
In @.github/workflows/install-script-test.yml:
- Around line 76-77: The CI step currently leaks secrets by grepping and
printing `.env` entries that include `database.default.password` and
`encryption.key`; update the command that reads `.env` (the line that currently
greps for
"database.default.(hostname|database|username|password)|encryption.key|CI_ENVIRONMENT")
to avoid emitting secret values—either stop matching the password and
encryption.key keys at all or pipe the matched lines through a redaction step so
only keys (or keys with a fixed "[REDACTED]" value) are printed; ensure the
change affects the command that also tails logs so CI output no longer contains
raw secret values.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 54def4db-d264-4953-b05c-e2f3f83f1a82
📒 Files selected for processing (2)
.github/workflows/build-release.yml.github/workflows/install-script-test.yml
Summary
Centralizes version tag generation logic into a shared script to fix inconsistencies and broken PR deployments.
Problem
build-release.yml:{VERSION}-{BRANCH}-{SHA-6}deploy-pr.yml:pr-{PR_NUMBER}-{SHA-7}deploy-pr.ymlexpected Docker images that didn't exist becausebuild-release.ymlonly built images on push events, not PR eventsChanges
.github/scripts/get-version.sh- shared version tag generation scriptbuild-release.yml- uses shared script, now builds Docker images for PRsdeploy-pr.yml- uses shared script for consistent taggingTag Formats (Standardized)
pr-{NUMBER}-{SHA-7}pr-123-abc1234{VERSION}(+latest)3.4.0,latest{VERSION}-{BRANCH}-{SHA-7}3.4.0-feature_foo-abc1234Testing
latesttagSummary by CodeRabbit
New Features
Documentation
Tests
Chores