Skip to content

fix(texlive-tags): tell a grep failure from zero matches #175

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
toshi0806 merged 1 commit into from
Sep 6, 2026
Merged

fix(texlive-tags): tell a grep failure from zero matches #175

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
16 changes: 14 additions & 2 deletions scripts/audit-texlive-tags.sh
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -127,10 +127,16 @@ while IFS= read -r spec; do
# からの相対パスなので、grep も repo root で走らせる
# cd の失敗は握り潰さない。空の結果は「一致している」と区別が付かず、
# 何も見ていない監査が ok を出すことになる
# grep の失敗は 0 件と区別する。終了コードでは見分けられない: マッチが
# 無いとき grep は 1 を返すが、xargs は子が 1..125 で終わると一律 123 を
# 返すため、「一致しなかった」と「起動できなかった」が同じ値になる。
# grep はマッチが無いだけなら標準エラーに何も書かないので、その有無で
# 判定する。捨ててしまうと、検出器が動いていない監査が ok を出す
: > "$err_file"

@github-actions github-actions Bot Sep 5, 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] : > "$err_file" でファイルをクリアしてから、extra_patterns のループ内で 2>>"$err_file" と追記しています。この設計では、ループの途中のイテレーションで grep が失敗しても、後続のイテレーションが成功すれば err_file にはエラーが残り続けるという点は問題ありません。しかし逆に、最初の hits= の grep が失敗した場合でも、extra_patterns ループが全て正常終了すれば err_file にはエラーが書き込まれたまま残るため、正しく検出されます。

ただし、より深刻な問題として、err_file が複数のリポジトリ間で共有されている場合、前のリポジトリの grep エラーが残っていると誤検知が起きます。: > "$err_file" のクリアはループの先頭（各リポジトリの処理開始時）に置かれているため、この点は正しく対処されています。

一方で、err_file の定義箇所（スクリプト上部）を確認し、mktemp 等で一時ファイルとして作成されているか確認してください。もし固定パスであれば、並列実行時に競合が生じます。スクリプトが並列実行される可能性がある場合は mktemp による一時ファイルの利用を推奨します。

@toshi0806 toshi0806 Sep 5, 2026

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

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

確認しました。mktemp で作成されています。 固定パスではないので並列実行での競合はありません。

workdir=$(mktemp -d)
err_file=$(mktemp)
trap 'rm -rf "$workdir" "$err_file"' EXIT

trap によるクリーンアップも入っています。ご指摘の推奨事項は既に満たされている形です。

前半で挙げていただいた 2 点についても、認識は一致しています。

  • 最初の hits= の grep が失敗し、後続の extra_patterns が正常終了しても、err_file の内容は残るので検出される
  • リポジトリ間の持ち越しは、各リポジトリの処理開始時に : > "$err_file" でクリアすることで防いでいる

なお : > "$err_file" を置いた位置には、もう 1 つ理由があります。clone が >"$err_file" 2>&1 で同じファイルに書くため、clone が成功しても何か出力していた場合にそれが残ります。grep の判定を始める前にクリアしないと、clone の出力を grep の失敗と誤認します。

コード変更は不要と判断しました。

hits=$(
cd "$repo_dir" || exit 1
git ls-files -z \
| xargs -0 -r grep -EoHn "${image}:[A-Za-z0-9._-]+" 2>/dev/null \
| xargs -0 -r grep -EoHn "${image}:[A-Za-z0-9._-]+" 2>>"$err_file" \
| sed "s|:${image}:|:|" || true
)

Expand All @@ -139,13 +145,19 @@ while IFS= read -r spec; do
[ -z "$pattern" ] && continue
found=$(
cd "$repo_dir" || exit 1
git ls-files -z | xargs -0 -r grep -oHnP "$pattern" 2>/dev/null || true
git ls-files -z | xargs -0 -r grep -oHnP "$pattern" 2>>"$err_file" || true
)
[ -n "$found" ] && hits=$(printf '%s\n%s' "$hits" "$found")
done <<EOF
$extra
EOF

if [ -s "$err_file" ]; then
errors=$((errors + 1))
log " ERROR: ${name} — grep が失敗した: $(tr '\n' ' ' < "$err_file" | cut -c1-200)"

@github-actions github-actions Bot Sep 5, 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.

ℹ️ [LOW] $(tr '\n' ' ' < "$err_file" | cut -c1-200) はコマンド置換内でファイルを読み込んでいますが、err_file に制御文字や特殊なバイト列が含まれる場合（悪意あるリポジトリ名やファイル名など）、ログ出力が意図しない形式になる可能性があります。実害は限定的ですが、cat "$err_file" | head -c 200 のように単純化するか、出力をクォートで保護することを検討してください。現状の log 関数がどのように実装されているかにも依存しますが、ログインジェクションのリスクとして認識しておくべきです。

@toshi0806 toshi0806 Sep 5, 2026

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

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

据え置きます。 理由は、この書き方が既存の ERROR 行と揃えたものだからです。

同じスクリプトの clone 失敗が、以前から同じ形を使っています。

log "  ERROR: ${name} — clone できなかった: $(tr '\n' ' ' < "$err_file")"

今回の行はこれに cut -c1-200 を足しただけです（grep のエラーは複数ファイル分が並んで長くなりうるため）。ここだけ制御文字を落とすと、同じファイル内の同種の 2 行で扱いが分かれます。 対処するならスクリプト全体、さらには audit-repo-protection.sh を含めた方針として決めるべきものです。

脅威モデルについても、実害は限定的だと考えています。

  • 監査対象は config/texlive-tag-refs.json に列挙した smkwlab org の 12 リポジトリで、ファイル名は自分たちが管理している
  • err_file に入るのは grep / xargs / gh のエラーメッセージで、任意の入力ではない
  • log() は echo "[$(date ...)] $1" で、-e を付けていないためバックスラッシュ列は解釈されない

残るリスクは、追跡ファイル名に生の ANSI エスケープを含むリポジトリを監査対象に加えた場合に CI ログの表示が乱れることですが、この監査は org 内の自リポジトリを対象にしたもので、そこまでの入力を想定していません。

もし方針として制御文字を落とすなら、tr -d '\000-\010\013\014\016-\037' のように範囲を限る必要があります。[:print:] で絞ると UTF-8 の継続バイトが巻き込まれ、日本語のファイル名やエラーメッセージが壊れます。このリポジトリのログは日本語なので、ここは慎重に決める必要があります。

continue
fi

repo_drift=0
repo_skipped=0
while IFS= read -r hit; do
Expand Down