From 9c5a2ed008cee788d5a289336035137d9f6ff3cc Mon Sep 17 00:00:00 2001 From: Yoshifumi Nakamura Date: Fri, 29 May 2026 08:26:39 +0900 Subject: [PATCH] Harden admin CLI email checks and shellcheck coverage Signed-off-by: Yoshifumi Nakamura --- .github/workflows/shellcheck.yml | 8 +++++-- docs/ci.md | 5 +++-- result_server/create_admin.py | 18 +++++++++------ result_server/tests/test_admin_hardening.py | 25 +++++++++++++++++++++ 4 files changed, 45 insertions(+), 11 deletions(-) diff --git a/.github/workflows/shellcheck.yml b/.github/workflows/shellcheck.yml index 15d0629..351616f 100644 --- a/.github/workflows/shellcheck.yml +++ b/.github/workflows/shellcheck.yml @@ -6,6 +6,8 @@ on: - "scripts/*.sh" - "scripts/**/*.sh" - "programs/**/*.sh" + - "benchpark-bridge/scripts/*.sh" + - "benchpark-bridge/scripts/**/*.sh" - ".github/workflows/shellcheck.yml" push: branches: @@ -14,6 +16,8 @@ on: - "scripts/*.sh" - "scripts/**/*.sh" - "programs/**/*.sh" + - "benchpark-bridge/scripts/*.sh" + - "benchpark-bridge/scripts/**/*.sh" - ".github/workflows/shellcheck.yml" workflow_dispatch: @@ -35,11 +39,11 @@ jobs: - name: Install shellcheck-py run: python -m pip install --quiet shellcheck-py - - name: Run shellcheck on scripts/ and programs/ at error level + - name: Run shellcheck on shell entrypoints at error level shell: bash run: | set -euo pipefail - mapfile -t files < <(find scripts/ programs/ -name "*.sh" 2>/dev/null | sort) + mapfile -t files < <(find scripts/ programs/ benchpark-bridge/scripts/ -name "*.sh" 2>/dev/null | sort) if [ "${#files[@]}" -eq 0 ]; then echo "No shell files found; nothing to check." exit 0 diff --git a/docs/ci.md b/docs/ci.md index 8b562c2..9d611fa 100644 --- a/docs/ci.md +++ b/docs/ci.md @@ -16,7 +16,7 @@ BenchKit uses GitHub as the public development repository and GitLab CI for benc | `Sync protected branches to GitLab` | Pushes to `develop` or `main` / `develop`または`main`へのpush | Mirrors protected branches to GitLab without starting GitLab CI / GitLab CIを発火させずに保護ブランチをGitLabへ同期する | | `Guard main PR source` | Pull requests to `main` / `main`宛PR | Allows only upstream `develop` to target `main` / upstreamの`develop`から`main`へのPRだけを許可する | | `Result Server Tests` | Result server, portal metadata, site config, or portal upload helper changes / result server、portal metadata、site config、portal upload helper関連変更 | Runs site config preflight and result server tests / site config preflightとresult serverのテストを実行する | -| `Shellcheck` | Shell script changes under `scripts/` or `programs/` / `scripts/`または`programs/`配下のshell script変更 | Gates shellcheck error-level issues only / shellcheckのerror級だけをgateする | +| `Shellcheck` | Shell script changes under `scripts/`, `programs/`, or `benchpark-bridge/scripts/` / `scripts/`、`programs/`、または`benchpark-bridge/scripts/`配下のshell script変更 | Gates shellcheck error-level issues only / shellcheckのerror級だけをgateする | ## GitLab Secrets / GitLab secret @@ -254,7 +254,7 @@ app support matrix、partial support、app entrypoint不足、`list.csv` 内の | `result_server/**/*` / `result_server/**/*` | `Result Server Tests` | Skipped by `.gitlab-ci.yml` rules / `.gitlab-ci.yml` rulesでskip | Portal regressions should be caught by lightweight Python tests / portal回帰はlightweight Python testで捕捉する | | Public site config or portal metadata `config/system.csv`, `config/queue.csv`, `config/system_info.csv` / 公開site configまたはportal表示メタデータ`config/system.csv`、`config/queue.csv`、`config/system_info.csv` | `Result Server Tests`, including site config preflight / site config preflightを含む`Result Server Tests` | `config/system.csv` and `config/queue.csv` run by `.gitlab-ci.yml`; `config/system_info.csv` is skipped / `config/system.csv`と`config/queue.csv`は`.gitlab-ci.yml`で実行、`config/system_info.csv`はskip | Public systems listed in `system_info.csv` must also exist in `system.csv` and reference a queue defined in `queue.csv` / `system_info.csv`に載せる公開systemは`system.csv`にも存在し、`queue.csv`定義済みqueueを参照する必要がある | | Portal upload or profile-data helper `scripts/bk_functions.sh`, `scripts/result.sh`, `scripts/result_server/**` / portal uploadまたはprofile-data helper `scripts/bk_functions.sh`、`scripts/result.sh`、`scripts/result_server/**` | `Result Server Tests` when covered by its path filter; `Shellcheck` for `.sh` changes / path filter対象なら`Result Server Tests`; `.sh`変更は`Shellcheck` | GitHub pull requests do not start GitLab by default; if a direct/manual GitLab pipeline is started, `scripts/**/*` is treated as benchmark-affecting and runs / GitHub pull requestでは既定でGitLabは起動しない。直接/手動GitLab pipelineを起動した場合、`scripts/**/*` はbenchmark影響ありとして実行される | These helpers shape result JSON / upload behavior. Use lightweight tests first, then start `GitLab Manual CI` when benchmark-side behavior needs validation / これらのhelperはResult JSONやupload挙動へ影響する。まずlightweight testで確認し、benchmark側挙動の検証が必要な場合は`GitLab Manual CI`を起動する | -| Benchmark app code or other shared scripts / benchmark appコードまたはその他の共通script | `Shellcheck` for `.sh` changes; otherwise normal GitHub review checks / `.sh`変更は`Shellcheck`; それ以外は通常のGitHub review check | Run through `GitLab Manual CI` when maintainer starts it / maintainerが`GitLab Manual CI`を起動した場合に実行 | Use `code` and `system` filters when broad validation is unnecessary / 広範な検証が不要なら`code`と`system`を指定する | +| Benchmark app, BenchPark bridge, or other shared scripts / benchmark app、BenchPark bridge、またはその他の共通script | `Shellcheck` for `.sh` changes; otherwise normal GitHub review checks / `.sh`変更は`Shellcheck`; それ以外は通常のGitHub review check | Run through `GitLab Manual CI` when maintainer starts it / maintainerが`GitLab Manual CI`を起動した場合に実行 | Use `code`, `system`, `benchpark`, or `park_only` filters when broad validation is unnecessary / 広範な検証が不要なら`code`、`system`、`benchpark`、`park_only`を指定する | | GitHub workflow/action `.github/**/*` / GitHub workflow/action `.github/**/*` | Workflow-specific checks when paths match / path一致時にworkflowごとのcheck | Skipped by `.gitlab-ci.yml` rules / `.gitlab-ci.yml` rulesでskip | GitHub workflow/action changes affect API-calling or sync control logic. Validate them on the GitHub side; they are pushed to GitLab with `ci.skip` during protected-branch sync / GitHub workflow/action変更はAPI呼び出しやsync制御に影響する。GitHub側で確認する。protected-branch syncでは`ci.skip`付きでGitLabへpushされる | | `.gitlab-ci.yml` / `.gitlab-ci.yml` | Normal GitHub review checks only / 通常のGitHub review checkのみ | Run through `GitLab Manual CI` when a maintainer needs to validate GitLab pipeline behavior / GitLab pipeline挙動の検証が必要な場合にmaintainerが`GitLab Manual CI`で実行 | This file defines GitLab benchmark pipeline behavior / このファイルはGitLab benchmark pipeline挙動を定義する | @@ -272,6 +272,7 @@ Use these examples when deciding whether to split a pull request or start GitLab | `config/system.csv` or `config/queue.csv` for a public system / 公開system向けの`config/system.csv`または`config/queue.csv` | `Result Server Tests` should run the site config preflight / `Result Server Tests`でsite config preflightを実行 | Start `GitLab Manual CI` too when benchmark execution behavior needs validation / benchmark実行挙動の検証が必要なら`GitLab Manual CI`も起動 | | `scripts/bk_functions.sh`, `scripts/result.sh`, or `scripts/result_server/**` only / `scripts/bk_functions.sh`、`scripts/result.sh`、または`scripts/result_server/**`のみ | `Result Server Tests` should run when the path filter matches; `Shellcheck` should run for `.sh` changes / path filter対象なら`Result Server Tests`が動く; `.sh`変更では`Shellcheck`が動く | Protected-branch sync uses `ci.skip`; direct/manual GitLab pipelines run because `.gitlab-ci.yml` treats `scripts/**/*` as benchmark-affecting / protected branch syncは`ci.skip`を使う。直接/手動GitLab pipelineでは`.gitlab-ci.yml`が`scripts/**/*`をbenchmark影響ありとして扱うため実行される | | `programs/qws/**/*` or `scripts/job/**/*` / `programs/qws/**/*`または`scripts/job/**/*` | `Shellcheck` should run for `.sh` changes / `.sh`変更では`Shellcheck`が動く | Start `GitLab Manual CI` when benchmark validation is needed, preferably with explicit `code` and `system` filters / benchmark検証が必要なら`code`と`system`を明示して`GitLab Manual CI`を起動 | +| `benchpark-bridge/scripts/**/*.sh` / `benchpark-bridge/scripts/**/*.sh` | `Shellcheck` should run / `Shellcheck`が動く | Start `GitLab Manual CI` with `benchpark=true` or `park_only=true` when BenchPark execution behavior needs validation / BenchPark実行挙動の検証が必要なら`benchpark=true`または`park_only=true`で`GitLab Manual CI`を起動 | | `.github/workflows/sync-to-gitlab.yml` or `.github/actions/prepare-gitlab-repo/action.yml` / `.github/workflows/sync-to-gitlab.yml`または`.github/actions/prepare-gitlab-repo/action.yml` | Validate on the GitHub Actions side / GitHub Actions側で確認 | Skipped by `.gitlab-ci.yml` rules when changed alone; protected-branch sync pushes it with `ci.skip` / 単独変更なら`.gitlab-ci.yml` rulesでskip。protected-branch syncでは`ci.skip`付きでpushされる | | `.gitlab-ci.yml` / `.gitlab-ci.yml` | Review the GitLab rule diff carefully / GitLab rule差分を慎重にreview | Start `GitLab Manual CI` if rule behavior itself needs validation / rule挙動そのものの検証が必要なら`GitLab Manual CI`を起動 | diff --git a/result_server/create_admin.py b/result_server/create_admin.py index adc4752..9f5c0f5 100644 --- a/result_server/create_admin.py +++ b/result_server/create_admin.py @@ -26,7 +26,7 @@ import redis -from utils.admin_policy import parse_affiliations, parse_allowed_affiliations +from utils.admin_policy import is_valid_email, parse_affiliations, parse_allowed_affiliations from utils.user_store import UserStore @@ -55,6 +55,10 @@ def main(): ) args = parser.parse_args() + email = args.email.strip() + if not is_valid_email(email): + parser.error("Invalid email address.") + allowed = parse_allowed_affiliations(os.environ.get("RESULT_SERVER_ALLOWED_AFFILIATIONS")) affiliations, invalid = parse_affiliations(args.affiliations, allowed) if invalid: @@ -64,17 +68,17 @@ def main(): store = UserStore(r_conn, args.prefix) # Reuse an existing account by clearing the current secret and issuing a new invite. - if store.user_exists(args.email): - print(f"User {args.email} already exists.") + if store.user_exists(email): + print(f"User {email} already exists.") ans = input("Generate reinvite link? [y/N]: ").strip().lower() if ans != "y": return - store.clear_totp_secret(args.email) - token = store.create_invitation(args.email, affiliations) + store.clear_totp_secret(email) + token = store.create_invitation(email, affiliations) else: - token = store.create_invitation(args.email, affiliations) + token = store.create_invitation(email, affiliations) - print(f"\nInvitation created for: {args.email}") + print(f"\nInvitation created for: {email}") print(f"Affiliations: {affiliations}") print("\nSetup URL:") print(f" {args.base_url}/auth/setup/{token}") diff --git a/result_server/tests/test_admin_hardening.py b/result_server/tests/test_admin_hardening.py index 669722e..24fe640 100644 --- a/result_server/tests/test_admin_hardening.py +++ b/result_server/tests/test_admin_hardening.py @@ -346,3 +346,28 @@ def test_create_admin_rejects_invalid_affiliation_before_redis(monkeypatch, caps assert excinfo.value.code == 2 assert "Invalid affiliations" in capsys.readouterr().err + + +def test_create_admin_rejects_invalid_email_before_redis(monkeypatch, capsys): + import create_admin + + def fail_if_called(*_args, **_kwargs): + raise AssertionError("Redis should not be contacted for invalid email") + + monkeypatch.setattr(create_admin.redis, "from_url", fail_if_called, raising=False) + monkeypatch.setattr( + sys, + "argv", + [ + "create_admin.py", + "evil';alert(1);//@x.com", + "--affiliations", + "admin", + ], + ) + + with pytest.raises(SystemExit) as excinfo: + create_admin.main() + + assert excinfo.value.code == 2 + assert "Invalid email address" in capsys.readouterr().err