Skip to content

Commit ca8277c

Browse files
Harden admin CLI email checks and shellcheck coverage
Signed-off-by: Yoshifumi Nakamura <nakamura@riken.jp>
1 parent 8c4aa17 commit ca8277c

4 files changed

Lines changed: 45 additions & 11 deletions

File tree

‎.github/workflows/shellcheck.yml‎

Lines changed: 6 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -6,6 +6,8 @@ on:
66
- "scripts/*.sh"
77
- "scripts/**/*.sh"
88
- "programs/**/*.sh"
9+
- "benchpark-bridge/scripts/*.sh"
10+
- "benchpark-bridge/scripts/**/*.sh"
911
- ".github/workflows/shellcheck.yml"
1012
push:
1113
branches:
@@ -14,6 +16,8 @@ on:
1416
- "scripts/*.sh"
1517
- "scripts/**/*.sh"
1618
- "programs/**/*.sh"
19+
- "benchpark-bridge/scripts/*.sh"
20+
- "benchpark-bridge/scripts/**/*.sh"
1721
- ".github/workflows/shellcheck.yml"
1822
workflow_dispatch:
1923

@@ -35,11 +39,11 @@ jobs:
3539
- name: Install shellcheck-py
3640
run: python -m pip install --quiet shellcheck-py
3741

38-
- name: Run shellcheck on scripts/ and programs/ at error level
42+
- name: Run shellcheck on shell entrypoints at error level
3943
shell: bash
4044
run: |
4145
set -euo pipefail
42-
mapfile -t files < <(find scripts/ programs/ -name "*.sh" 2>/dev/null | sort)
46+
mapfile -t files < <(find scripts/ programs/ benchpark-bridge/scripts/ -name "*.sh" 2>/dev/null | sort)
4347
if [ "${#files[@]}" -eq 0 ]; then
4448
echo "No shell files found; nothing to check."
4549
exit 0

‎docs/ci.md‎

Lines changed: 3 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -16,7 +16,7 @@ BenchKit uses GitHub as the public development repository and GitLab CI for benc
1616
| `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へ同期する |
1717
| `Guard main PR source` | Pull requests to `main` / `main`宛PR | Allows only upstream `develop` to target `main` / upstreamの`develop`から`main`へのPRだけを許可する |
1818
| `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のテストを実行する |
19-
| `Shellcheck` | Shell script changes under `scripts/` or `programs/` / `scripts/`または`programs/`配下のshell script変更 | Gates shellcheck error-level issues only / shellcheckのerror級だけをgateする |
19+
| `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する |
2020

2121
## GitLab Secrets / GitLab secret
2222

@@ -254,7 +254,7 @@ app support matrix、partial support、app entrypoint不足、`list.csv` 内の
254254
| `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で捕捉する |
255255
| 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を参照する必要がある |
256256
| 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`を起動する |
257-
| 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`を指定する |
257+
| 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`を指定する |
258258
| 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される |
259259
| `.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挙動を定義する |
260260

@@ -272,6 +272,7 @@ Use these examples when deciding whether to split a pull request or start GitLab
272272
| `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`も起動 |
273273
| `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影響ありとして扱うため実行される |
274274
| `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`を起動 |
275+
| `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`を起動 |
275276
| `.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される |
276277
| `.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`を起動 |
277278

‎result_server/create_admin.py‎

Lines changed: 11 additions & 7 deletions
Original file line numberDiff line numberDiff line change
@@ -26,7 +26,7 @@
2626

2727
import redis
2828

29-
from utils.admin_policy import parse_affiliations, parse_allowed_affiliations
29+
from utils.admin_policy import is_valid_email, parse_affiliations, parse_allowed_affiliations
3030
from utils.user_store import UserStore
3131

3232

@@ -55,6 +55,10 @@ def main():
5555
)
5656
args = parser.parse_args()
5757

58+
email = args.email.strip()
59+
if not is_valid_email(email):
60+
parser.error("Invalid email address.")
61+
5862
allowed = parse_allowed_affiliations(os.environ.get("RESULT_SERVER_ALLOWED_AFFILIATIONS"))
5963
affiliations, invalid = parse_affiliations(args.affiliations, allowed)
6064
if invalid:
@@ -64,17 +68,17 @@ def main():
6468
store = UserStore(r_conn, args.prefix)
6569

6670
# Reuse an existing account by clearing the current secret and issuing a new invite.
67-
if store.user_exists(args.email):
68-
print(f"User {args.email} already exists.")
71+
if store.user_exists(email):
72+
print(f"User {email} already exists.")
6973
ans = input("Generate reinvite link? [y/N]: ").strip().lower()
7074
if ans != "y":
7175
return
72-
store.clear_totp_secret(args.email)
73-
token = store.create_invitation(args.email, affiliations)
76+
store.clear_totp_secret(email)
77+
token = store.create_invitation(email, affiliations)
7478
else:
75-
token = store.create_invitation(args.email, affiliations)
79+
token = store.create_invitation(email, affiliations)
7680

77-
print(f"\nInvitation created for: {args.email}")
81+
print(f"\nInvitation created for: {email}")
7882
print(f"Affiliations: {affiliations}")
7983
print("\nSetup URL:")
8084
print(f" {args.base_url}/auth/setup/{token}")

‎result_server/tests/test_admin_hardening.py‎

Lines changed: 25 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -346,3 +346,28 @@ def test_create_admin_rejects_invalid_affiliation_before_redis(monkeypatch, caps
346346

347347
assert excinfo.value.code == 2
348348
assert "Invalid affiliations" in capsys.readouterr().err
349+
350+
351+
def test_create_admin_rejects_invalid_email_before_redis(monkeypatch, capsys):
352+
import create_admin
353+
354+
def fail_if_called(*_args, **_kwargs):
355+
raise AssertionError("Redis should not be contacted for invalid email")
356+
357+
monkeypatch.setattr(create_admin.redis, "from_url", fail_if_called, raising=False)
358+
monkeypatch.setattr(
359+
sys,
360+
"argv",
361+
[
362+
"create_admin.py",
363+
"evil';alert(1);//@x.com",
364+
"--affiliations",
365+
"admin",
366+
],
367+
)
368+
369+
with pytest.raises(SystemExit) as excinfo:
370+
create_admin.main()
371+
372+
assert excinfo.value.code == 2
373+
assert "Invalid email address" in capsys.readouterr().err

0 commit comments

Comments
 (0)