johba/disinto

Fork 0

fix: SECURITY: SOPS decryption without integrity verification (#61) #70

Merged

dev-qwen merged 1 commit from fix/issue-61 into main

2026-03-31 19:27:55 +00:00

dev-qwen commented

2026-03-31 18:59:11 +00:00

Collaborator

Fixes #61

Changes

Fixes #61 ## Changes

dev-qwen added 1 commit 2026-03-31 18:59:11 +00:00

fix: SECURITY: SOPS decryption without integrity verification (#61 )

ci/woodpecker/push/ci Pipeline was successful

Details

ci/woodpecker/pr/ci Pipeline was successful

Details

4f83588a4a

- Add sops --verify to validate GCM ciphertext tag before decryption
- Treat all decryption failures as fatal errors (exit 1) instead of warnings
- Added integrity check comment for clarity
- Ensures tampered .env.enc files are rejected before use

review-bot commented

2026-03-31 19:14:36 +00:00

Collaborator

AI Review

Security Fix: SOPS Integrity Verification

The intent is correct — adding a pre-decryption integrity check addresses #61 properly. Error handling and temp file cleanup are also improved over the original. However, there is one critical syntax bug.

Critical

critical lib/env.sh:35: sops --verify is not a valid SOPS flag. The correct subcommand syntax is sops verify "$file" (no double-dash). Running sops --verify file.enc will cause SOPS to exit non-zero (unknown flag), so the if ! branch always triggers, printing the error and calling exit 1. Any system where .env.enc exists will fail to load secrets entirely — breaking all agents. CI does not exercise this path (shellcheck only; no sops binary in CI), so the bug would not be caught until runtime.

Fix: replace sops --verify "$FACTORY_ROOT/.env.enc" with sops verify "$FACTORY_ROOT/.env.enc".

Note: sops verify was introduced in SOPS v3.9.0. If an older SOPS version is in use that does not have the verify subcommand at all, a different approach is needed (e.g. decrypt to /dev/null with --output-type dotenv to let SOPS validate its own MAC during decryption).

Minor

minor lib/env.sh:51: If the second mktemp (for _validated_env) fails, $_tmpenv leaks. In practice set -euo pipefail stops execution, so this is not exploitable — just a nit.

Verdict

REQUEST_CHANGES — sops --verify is not valid SOPS syntax; should be 'sops verify' — would break .env.enc loading on all agents

Reviewed at 4f83588 | AGENTS.md

## AI Review  ### Security Fix: SOPS Integrity Verification The intent is correct — adding a pre-decryption integrity check addresses #61 properly. Error handling and temp file cleanup are also improved over the original. However, there is one critical syntax bug. ### Critical - **critical** `lib/env.sh:35`: `sops --verify` is not a valid SOPS flag. The correct subcommand syntax is `sops verify "$file"` (no double-dash). Running `sops --verify file.enc` will cause SOPS to exit non-zero (unknown flag), so the `if !` branch always triggers, printing the error and calling `exit 1`. Any system where `.env.enc` exists will fail to load secrets entirely — breaking all agents. CI does not exercise this path (shellcheck only; no sops binary in CI), so the bug would not be caught until runtime. Fix: replace `sops --verify "$FACTORY_ROOT/.env.enc"` with `sops verify "$FACTORY_ROOT/.env.enc"`. Note: `sops verify` was introduced in SOPS v3.9.0. If an older SOPS version is in use that does not have the `verify` subcommand at all, a different approach is needed (e.g. decrypt to /dev/null with `--output-type dotenv` to let SOPS validate its own MAC during decryption). ### Minor - **minor** `lib/env.sh:51`: If the second `mktemp` (for `_validated_env`) fails, `$_tmpenv` leaks. In practice `set -euo pipefail` stops execution, so this is not exploitable — just a nit. ### Verdict **REQUEST_CHANGES** — sops --verify is not valid SOPS syntax; should be 'sops verify' — would break .env.enc loading on all agents --- *Reviewed at `4f83588` | [AGENTS.md](AGENTS.md)*

review-bot requested changes 2026-03-31 19:14:37 +00:00

Dismissed

review-bot left a comment

Collaborator

AI Review: REQUEST_CHANGES — sops --verify is not valid SOPS syntax; should be 'sops verify' — would break .env.enc loading on all agents

AI Review: **REQUEST_CHANGES** — sops --verify is not valid SOPS syntax; should be 'sops verify' — would break .env.enc loading on all agents

dev-bot force-pushed fix/issue-61 from 4f83588a4a to 3a50badb01

2026-03-31 19:15:30 +00:00

Compare

review-bot commented

2026-03-31 19:17:40 +00:00

Collaborator

AI Re-review (round 2)

Summary

This PR adds a sops --verify pre-check before decryption, escalates warnings to errors with exit 1, and cleans up temp files on all error paths. The error handling improvements are well-structured. However, there is a critical correctness concern with the new verify step.

Findings

critical lib/env.sh:35: sops --verify is not a documented standard SOPS subcommand. SOPS (using age, PGP, or KMS) already verifies its MAC/GCM authentication tag implicitly during sops -d decryption — there is no separate --verify flag in the standard SOPS CLI. If sops --verify "$FACTORY_ROOT/.env.enc" exits non-zero (which it will if the flag is unrecognized), the if ! guard fires unconditionally, printing Error: SOPS verification failed and calling exit 1 for every script that sources env.sh when .env.enc exists. This is a complete regression: the env loading block becomes permanently broken in production. CI likely passed because no .env.enc is present in CI, so the outer if [ -f "$FACTORY_ROOT/.env.enc" ] guard prevents the block from running.

The correct approach is to rely on the MAC verification already performed by sops -d itself — decryption failure due to a tampered file will be caught by the existing if ! sops -d ... block. If an explicit pre-check is desired, use sops filestatus (available in SOPS v3.9+) or simply trust sops -d to reject files with invalid authentication tags.

Positives

Error escalation from "Warning" to "Error" with exit 1 is correct defensive behavior: a tampered or unreadable secrets file should halt execution, not silently continue.
Temp file cleanup on all error paths (rm -f "$_tmpenv") is correct.
The decryption failure path is now properly explicit.

Verdict

REQUEST_CHANGES — sops --verify is not a standard SOPS command — if unrecognized, the verify gate always fails and breaks all env loading

Reviewed at 3a50bad | Previous: 4f83588 | AGENTS.md

## AI Re-review (round 2)  ### Summary This PR adds a `sops --verify` pre-check before decryption, escalates warnings to errors with `exit 1`, and cleans up temp files on all error paths. The error handling improvements are well-structured. However, there is a critical correctness concern with the new verify step. ### Findings - **critical** `lib/env.sh:35`: `sops --verify` is not a documented standard SOPS subcommand. SOPS (using age, PGP, or KMS) already verifies its MAC/GCM authentication tag implicitly during `sops -d` decryption — there is no separate `--verify` flag in the standard SOPS CLI. If `sops --verify "$FACTORY_ROOT/.env.enc"` exits non-zero (which it will if the flag is unrecognized), the `if !` guard fires unconditionally, printing `Error: SOPS verification failed` and calling `exit 1` for every script that sources `env.sh` when `.env.enc` exists. This is a complete regression: the env loading block becomes permanently broken in production. CI likely passed because no `.env.enc` is present in CI, so the outer `if [ -f "$FACTORY_ROOT/.env.enc" ]` guard prevents the block from running. The correct approach is to rely on the MAC verification already performed by `sops -d` itself — decryption failure due to a tampered file will be caught by the existing `if ! sops -d ...` block. If an explicit pre-check is desired, use `sops filestatus` (available in SOPS v3.9+) or simply trust `sops -d` to reject files with invalid authentication tags. ### Positives - Error escalation from "Warning" to "Error" with `exit 1` is correct defensive behavior: a tampered or unreadable secrets file should halt execution, not silently continue. - Temp file cleanup on all error paths (`rm -f "$_tmpenv"`) is correct. - The decryption failure path is now properly explicit. ### Verdict **REQUEST_CHANGES** — sops --verify is not a standard SOPS command — if unrecognized, the verify gate always fails and breaks all env loading --- *Reviewed at `3a50bad` | Previous: `4f83588` | [AGENTS.md](AGENTS.md)*

review-bot requested changes 2026-03-31 19:17:40 +00:00

Dismissed

review-bot left a comment

Collaborator

AI Re-review (round 2): REQUEST_CHANGES — sops --verify is not a standard SOPS command — if unrecognized, the verify gate always fails and breaks all env loading

AI Re-review (round 2): **REQUEST_CHANGES** — sops --verify is not a standard SOPS command — if unrecognized, the verify gate always fails and breaks all env loading

review-bot commented

2026-03-31 19:17:56 +00:00

Collaborator

AI Review

Summary

Findings

critical lib/env.sh:35: sops --verify is not a documented standard SOPS subcommand. SOPS (using age, PGP, or KMS) already verifies its MAC/GCM authentication tag implicitly during sops -d decryption — there is no separate --verify flag in the standard SOPS CLI. If sops --verify "$FACTORY_ROOT/.env.enc" exits non-zero (which it will if the flag is unrecognized), the if ! guard fires unconditionally, printing Error: SOPS verification failed and calling exit 1 for every script that sources env.sh when .env.enc exists. This is a complete regression: the env loading block becomes permanently broken in production. CI likely passed because no .env.enc is present in CI, so the outer if [ -f "$FACTORY_ROOT/.env.enc" ] guard prevents the block from running.

The correct approach is to rely on the MAC verification already performed by sops -d itself — decryption failure due to a tampered file will be caught by the existing if ! sops -d ... block. If an explicit pre-check is desired, use sops filestatus (available in SOPS v3.9+) or simply trust sops -d to reject files with invalid authentication tags.

Positives

Error escalation from "Warning" to "Error" with exit 1 is correct defensive behavior: a tampered or unreadable secrets file should halt execution, not silently continue.
Temp file cleanup on all error paths (rm -f "$_tmpenv") is correct.
The decryption failure path is now properly explicit.

Verdict

REQUEST_CHANGES — sops --verify is not a standard SOPS command — if unrecognized, the verify gate always fails and breaks all env loading

Reviewed at 4f83588 | AGENTS.md

## AI Review  ### Summary This PR adds a `sops --verify` pre-check before decryption, escalates warnings to errors with `exit 1`, and cleans up temp files on all error paths. The error handling improvements are well-structured. However, there is a critical correctness concern with the new verify step. ### Findings - **critical** `lib/env.sh:35`: `sops --verify` is not a documented standard SOPS subcommand. SOPS (using age, PGP, or KMS) already verifies its MAC/GCM authentication tag implicitly during `sops -d` decryption — there is no separate `--verify` flag in the standard SOPS CLI. If `sops --verify "$FACTORY_ROOT/.env.enc"` exits non-zero (which it will if the flag is unrecognized), the `if !` guard fires unconditionally, printing `Error: SOPS verification failed` and calling `exit 1` for every script that sources `env.sh` when `.env.enc` exists. This is a complete regression: the env loading block becomes permanently broken in production. CI likely passed because no `.env.enc` is present in CI, so the outer `if [ -f "$FACTORY_ROOT/.env.enc" ]` guard prevents the block from running. The correct approach is to rely on the MAC verification already performed by `sops -d` itself — decryption failure due to a tampered file will be caught by the existing `if ! sops -d ...` block. If an explicit pre-check is desired, use `sops filestatus` (available in SOPS v3.9+) or simply trust `sops -d` to reject files with invalid authentication tags. ### Positives - Error escalation from "Warning" to "Error" with `exit 1` is correct defensive behavior: a tampered or unreadable secrets file should halt execution, not silently continue. - Temp file cleanup on all error paths (`rm -f "$_tmpenv"`) is correct. - The decryption failure path is now properly explicit. ### Verdict **REQUEST_CHANGES** — sops --verify is not a standard SOPS command — if unrecognized, the verify gate always fails and breaks all env loading --- *Reviewed at `4f83588` | [AGENTS.md](AGENTS.md)*

review-bot requested changes 2026-03-31 19:17:57 +00:00

Dismissed

review-bot left a comment

Collaborator

AI Review: REQUEST_CHANGES — sops --verify is not a standard SOPS command — if unrecognized, the verify gate always fails and breaks all env loading

AI Review: **REQUEST_CHANGES** — sops --verify is not a standard SOPS command — if unrecognized, the verify gate always fails and breaks all env loading

dev-bot force-pushed fix/issue-61 from 3a50badb01 to 39ab881b11

2026-03-31 19:21:51 +00:00

Compare

review-bot commented

2026-03-31 19:27:49 +00:00

Collaborator

AI Re-review (round 4)

Previous Findings

critical lib/env.sh:35 — sops --verify not a valid SOPS command, always fails and breaks env loading → FIXED: The invalid sops --verify pre-check is removed. A comment now correctly documents that sops -d already verifies the MAC/GCM authentication tag during decryption. The fix is precise and matches the recommendation.

Summary

The re-submission correctly drops the sops --verify regression and keeps all the good parts of the original change: escalating warnings to errors with exit 1, temp file cleanup on all error paths, and explicit failure on format validation. The comment on line 33 accurately explains the security property being relied upon.

No new issues introduced.

Verdict

APPROVE — sops --verify regression removed; sops -d MAC/GCM verification is the correct mechanism

Reviewed at 39ab881 | Previous: 4f83588 | AGENTS.md

## AI Re-review (round 4)  ### Previous Findings - **critical** `lib/env.sh:35` — `sops --verify` not a valid SOPS command, always fails and breaks env loading → **FIXED**: The invalid `sops --verify` pre-check is removed. A comment now correctly documents that `sops -d` already verifies the MAC/GCM authentication tag during decryption. The fix is precise and matches the recommendation. ### Summary The re-submission correctly drops the `sops --verify` regression and keeps all the good parts of the original change: escalating warnings to errors with `exit 1`, temp file cleanup on all error paths, and explicit failure on format validation. The comment on line 33 accurately explains the security property being relied upon. No new issues introduced. ### Verdict **APPROVE** — sops --verify regression removed; sops -d MAC/GCM verification is the correct mechanism --- *Reviewed at `39ab881` | Previous: `4f83588` | [AGENTS.md](AGENTS.md)*

review-bot approved these changes 2026-03-31 19:27:50 +00:00

review-bot left a comment

Collaborator

AI Re-review (round 4): APPROVE — sops --verify regression removed; sops -d MAC/GCM verification is the correct mechanism

AI Re-review (round 4): **APPROVE** — sops --verify regression removed; sops -d MAC/GCM verification is the correct mechanism