fix: require admin auth for glitch controls

[saim256]

Summary

• add default-deny admin authentication for Glitch API destructive/configuration routes
• require matching GLITCH_ADMIN_KEY via X-Admin-Key or X-API-Key
• protect history clear, config update/reset, enable, disable, and manual trigger endpoints
• add Flask route tests for wrong-key, unconfigured-key, and valid-key behavior

Fixes #4874

/claim #4874

Wallet: RTC253255d034065a839cd421811ec589ae5b694ffc

Validation

• python -m pytest issue2288\glitch_system\tests\test_glitch_system.py -q -> 43 passed
• python -m py_compile issue2288\glitch_system\src\api.py issue2288\glitch_system\tests\test_glitch_system.py -> passed
• git diff --check -> passed
• python tools\bcos_spdx_check.py --base-ref origin/main -> OK

[saim256]

Pushed follow-up commit \�09a88c\ to preserve the existing malformed-JSON validation behavior on the two JSON mutating routes before the admin gate.\n\nLocal verification after the follow-up:\n- \python -m pytest issue2288\glitch_system\tests\test_glitch_system.py tests\test_glitch_api_input_validation.py -q\ -> 52 passed\n- \python -m py_compile issue2288\glitch_system\src\api.py issue2288\glitch_system\tests\test_glitch_system.py tests\test_glitch_api_input_validation.py\ -> passed\n- \git diff --check\ -> passed\n- \python tools\bcos_spdx_check.py --base-ref origin/main\ -> OK\n\nThe main CI test job is green after this update; only the BCOS engine scan was still running at last check.

[jaxint]

Great contribution! LGTM.

[jaxint]

Great contribution! LGTM.

[idontwantagoldfish83]

I found one blocking auth edge case in the new guard.

require_admin() calls hmac.compare_digest(provided_key, expected_key) on user-controlled header strings. For str inputs, Python raises TypeError when either value contains non-ASCII characters, so a malformed admin header returns a 500 instead of the same 401 used for missing or wrong keys.

I reproduced this on the PR branch with GLITCH_ADMIN_KEY=test-admin:

resp = client.post("/api/glitch/disable", headers={"X-Admin-Key": "é"})

Actual result: Flask logs TypeError: comparing strings with non-ASCII characters is not supported and returns 500 Internal Server Error. This affects every route protected by require_admin(), since they all pass through the same comparison.

Expected result: malformed/non-matching credentials should be treated as unauthorized and return the normal 401 response without surfacing an unhandled exception.

A durable fix would be to normalize both values to bytes before the constant-time comparison, for example:

provided_key_bytes = provided_key.encode("utf-8")
expected_key_bytes = expected_key.encode("utf-8")
if not hmac.compare_digest(provided_key_bytes, expected_key_bytes):
    ...

Alternatively, catch TypeError around the comparison and return the same unauthorized response, but byte normalization keeps the path simple and preserves constant-time comparison semantics for Unicode-capable secrets too. Please add a regression test that sends a non-ASCII X-Admin-Key and asserts a 401.

Validation I ran:

• .venv/bin/python -m pytest issue2288/glitch_system/tests/test_glitch_system.py::TestGlitchAPIAdminAuth tests/test_glitch_api_input_validation.py -q -> 12 passed
• .venv/bin/python -m pytest issue2288/glitch_system/tests/test_glitch_system.py tests/test_glitch_api_input_validation.py -q -> 51 passed, 1 existing stochastic failure in TestGlitchEngineIntegration.test_conversation_flow

[loganoe]

Approved. This secures the correct issue2288/glitch_system/src/api.py mutating routes with a default-deny admin key while preserving the existing JSON validation behavior for the routes that parse request bodies.

Validation performed:

• git diff --check origin/main...HEAD
• python3 -m py_compile issue2288/glitch_system/src/api.py issue2288/glitch_system/tests/test_glitch_system.py
• PYTEST_DISABLE_PLUGIN_AUTOLOAD=1 /tmp/rustchain-flask-venv/bin/python -m pytest -q issue2288/glitch_system/tests/test_glitch_system.py -> 43 passed, 6 subtests passed

[galpetame]

Review for #4884

The patch is focused and the existing test suite passes locally (43 passed, 6 subtests). I found one auth-boundary issue worth tightening before merge.

Two admin-only routes still parse and validate JSON before the admin check:

• PUT /api/glitch/config: get_json_object() runs before require_admin().
• POST /api/glitch/trigger: get_json_object() runs before require_admin().

Local probe against the current PR head:

• PUT /api/glitch/config with string JSON + wrong admin key -> 400
• POST /api/glitch/trigger with string JSON + wrong admin key -> 400
• POST /api/glitch/disable with valid X-API-Key alias -> 200

This does not allow mutation without the admin key, but it means wrong-key/unauthenticated callers can still reach request-body validation on two routes and get 400 JSON object required before the admin gate. For a default-deny admin boundary, I would expect those requests to fail with 401 before any body validation.

Suggested fix:

1. Move require_admin() before get_json_object() in update_config() and trigger_glitch().
2. Add a regression where wrong/missing admin key plus non-object JSON returns 401.
3. If X-API-Key remains supported as an alias, add one positive alias test so that behavior is intentional and covered.

Validation I ran:

• git diff --stat origin/main HEAD -> 2 files changed, 109 insertions(+), 4 deletions(-)
• git diff --check origin/main HEAD -- issue2288/glitch_system/src/api.py issue2288/glitch_system/tests/test_glitch_system.py -> passed
• python -m py_compile issue2288/glitch_system/src/api.py issue2288/glitch_system/tests/test_glitch_system.py -> passed
• python -m pytest issue2288/glitch_system/tests/test_glitch_system.py -q -> 43 passed, 6 subtests passed

GitHub handle for tagging: @galpetame

Bounty #73 payout wallet if this review is eligible: RTCe4fbe4c9085b8b2ed3f1228504de66799025f6ce

[saim256]

Pushed follow-up commit 580dbda for the non-ASCII admin-key blocker.

What changed:

• require_admin() now compares UTF-8 encoded bytes with hmac.compare_digest(), so malformed/non-ASCII header values are treated as unauthorized instead of raising TypeError.
• Added a regression that sends a non-ASCII X-Admin-Key to /api/glitch/disable and asserts the normal 401 unauthorized response.

Validation after the follow-up:

• python -m pytest issue2288\glitch_system\tests\test_glitch_system.py::TestGlitchAPIAdminAuth tests\test_glitch_api_input_validation.py -q -> 13 passed
• python -m pytest issue2288\glitch_system\tests\test_glitch_system.py tests\test_glitch_api_input_validation.py -q -> 53 passed on rerun; the first full-suite attempt hit the existing stochastic test_conversation_flow zero-glitch failure, then passed unchanged on rerun
• python -m py_compile issue2288\glitch_system\src\api.py issue2288\glitch_system\tests\test_glitch_system.py tests\test_glitch_api_input_validation.py -> passed
• git diff --check -> passed
• python tools\bcos_spdx_check.py --base-ref origin/main -> OK

[508704820]

Code Review: Require Admin Auth for Glitch Controls

Summary

Fixes #4877's default-allow vulnerability. Now implements default-deny: when GLITCH_ADMIN_KEY is not configured, returns 401 instead of allowing all access.

What Works Well

1. Default-deny: Fixed the critical vulnerability from #4877
2. UTF-8 encoding: encode("utf-8") before hmac.compare_digest — more robust
3. Clear error messages: "GLITCH_ADMIN_KEY not configured" vs "Invalid admin key"

This is the correct fix for #4877

I previously requested changes on #4877 for the default-allow pattern. This PR adopts the correct default-deny approach from #4882.

Remaining concern

Env var naming inconsistency: GLITCH_ADMIN_KEY, MEMORY_ADMIN_KEY, ADMIN_KEY across different PRs. A unified RUSTCHAIN_ADMIN_KEY would be cleaner.

Verdict: Approve ✅

This fixes the critical auth bypass I flagged in #4877.

[loganoe]

Updating my earlier approval: changes are needed. The admin guard passes Flask header strings directly into hmac.compare_digest(). Python raises TypeError when either string contains non-ASCII characters, so a malformed X-Admin-Key can turn the intended 401 into a 500.

Repro:

>>> import hmac
>>> hmac.compare_digest('é', 'secret')
TypeError: comparing strings with non-ASCII characters is not supported

Please normalize to bytes before comparison, for example by encoding both values with UTF-8 or by rejecting non-ASCII input before calling compare_digest(), and add a regression for a non-ASCII admin header.

[saim256]

Clarification on the current head 580dbda: this exact non-ASCII compare_digest() issue is already fixed in this commit.

Current code now compares encoded bytes:

if not hmac.compare_digest(
    provided_key.encode("utf-8"),
    expected_key.encode("utf-8"),
):

The regression added in the same commit sends X-Admin-Key: ? to /api/glitch/disable and asserts 401 unauthorized, not 500.

Validation after the fix:

• python -m pytest issue2288\glitch_system\tests\test_glitch_system.py::TestGlitchAPIAdminAuth tests\test_glitch_api_input_validation.py -q -> 13 passed
• python -m pytest issue2288\glitch_system\tests\test_glitch_system.py tests\test_glitch_api_input_validation.py -q -> 53 passed on rerun
• python -m py_compile issue2288\glitch_system\src\api.py issue2288\glitch_system\tests\test_glitch_system.py tests\test_glitch_api_input_validation.py -> passed
• git diff --check -> passed
• python tools\bcos_spdx_check.py --base-ref origin/main -> OK

So the requested change is already addressed on the current branch head.

[idontwantagoldfish83]

Follow-up on current head 580dbda: the blocker I requested changes for is resolved.

I verified require_admin() now encodes both the provided and expected admin keys to UTF-8 bytes before hmac.compare_digest(), so a non-ASCII X-Admin-Key follows the normal unauthorized path instead of raising TypeError. The new regression also covers this case.

Validation I ran on current head:

• .venv/bin/python -m pytest issue2288/glitch_system/tests/test_glitch_system.py::TestGlitchAPIAdminAuth tests/test_glitch_api_input_validation.py -q -> 13 passed
• .venv/bin/python -m py_compile issue2288/glitch_system/src/api.py issue2288/glitch_system/tests/test_glitch_system.py tests/test_glitch_api_input_validation.py -> passed

[shuibui]

Code Review: Solid Fix

Approve. Security pattern is correct.

**Verdict: Approve.

[shuibui]

Code Review: Approve

Good fix.

**Verdict: Approve.

[shuibui]

Code Review: Approve

Good fix. Addresses the issue correctly.

**Verdict: Approve.

[shuibui]

Code Review: Approve

Good fix. Addresses the issue correctly.

**Verdict: Approve.

[shuibui]

Code Review: Approve

Good fix.

**Verdict: Approve.

[TJCurnutte]

Approved after a focused review of this PR's diff against origin/main.

Scope checked:

• PR title: fix: require admin auth for glitch controls
• Changed files: issue2288/glitch_system/src/api.py, issue2288/glitch_system/tests/test_glitch_system.py
• Review finding: Reviewed issue2288/glitch_system/src/api.py, issue2288/glitch_system/tests/test_glitch_system.py; diff adds auth/admin guard, input validation, regression test coverage, security hardening, CI/workflow gate.

Validation run locally from a clean checkout of pull/4884/head:

• git diff --check origin/main...HEAD → exit 0 (no output)
• python3 tools/bcos_spdx_check.py --base-ref origin/main → exit 0 (BCOS SPDX check: OK)
• python3 -B -m py_compile issue2288/glitch_system/src/api.py issue2288/glitch_system/tests/test_glitch_system.py → exit 0 (no output)
• python3 -B -m pytest issue2288/glitch_system/tests/test_glitch_system.py -q → exit 0 (44 passed in 0.36s)
• Added-line secret-pattern scan → pass

I did not find a merge-blocking issue in this focused pass. The change is small enough to review directly, and the targeted gates above passed.

[himanalot]

Reviewed issue2288/glitch_system/src/api.py and the added Flask route tests. The destructive/configuration controls now require GLITCH_ADMIN_KEY via X-Admin-Key or X-API-Key, fail closed when the key is unset, and cover wrong-key, unconfigured-key, non-ASCII header, and valid-key behavior. The protected routes do not mutate state before successful auth.

I do not see a blocking issue in this patch. Approved.

[guangningsun]

PR Review — Standard Quality ✓

PR: #4884 — Fix: require admin auth for glitch controls

What I reviewed

• issue2288/glitch_system/src/api.py
• issue2288/glitch_system/tests/test_glitch_system.py

Observations

1. Adding import hmac and proper admin authentication to glitch system API prevents unauthorized manipulation of glitch parameters. Glitch system likely controls hardware timing or power settings — unauthorized access could damage equipment or disrupt mining.

2. hmac module for request authentication provides constant-time comparison to prevent timing attacks on the admin key.

3. New test with unittest.mock.patch validates admin-only access patterns.

LGTM.

Bounty: #2782
Disclosure: I received RTC compensation for this review.