fix: persist gateway signing secret so pairing/approval survive restarts - #3781
fix: persist gateway signing secret so pairing/approval survive restarts#3781praisonai-triage-agent[bot] wants to merge 2 commits into
Conversation
…tons survive restarts (fixes #3779) Route the callback-button HMAC secret (bots/_pairing_ui.py) through the same auto-provisioned, owner-only persisted secret used by the pairing code path, so approve/deny buttons keep verifying across process restarts with zero operator action. Explicit PRAISONAI_CALLBACK_SECRET override still wins. Also collapse the stale ephemeral _get_secret() onto the persisted store and correct the doctor check message. Co-authored-by: MervinPraison <MervinPraison@users.noreply.github.com>
|
@coderabbitai review |
|
/review |
✅ Action performedReview finished.
|
MervinPraison
added
pipeline/blocked:ci
Qodo reviews are paused for this user.Troubleshooting steps vary by plan Learn more → On a Teams plan? Using GitHub Enterprise Server, GitLab Self-Managed, or Bitbucket Data Center? |
|
Important Review skippedBot user detected. To trigger a single review, invoke the ⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Pro Plus Run ID: You can disable this status message by setting the Use the checkbox below for a quick retry:
📝 WalkthroughWalkthroughGateway pairing and callback HMAC secrets now use an explicit environment override or a persisted gateway secret. The gateway creates and reuses the secret across restarts. Doctor guidance now describes this behavior and cross-host configuration. ChangesGateway secret persistence
Estimated code review effort: 2 (Simple) | ~10 minutes Possibly related PRs
Suggested reviewers: 🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
Greptile SummaryThe PR unifies callback and pairing-code signing around the persisted gateway secret while retaining explicit environment overrides.
Confidence Score: 5/5The PR appears safe to merge because no blocking failure remains within the eligible follow-up-review scope. No blocking failure remains.
|
| Filename | Overview |
|---|---|
| src/praisonai-bot/praisonai_bot/bots/_pairing_ui.py | Callback signing now uses the persisted gateway secret by default while preserving the callback-specific override and a logged degraded fallback. |
| src/praisonai-bot/praisonai_bot/gateway/pairing.py | Pairing secret resolution now consistently uses the persisted store, with fail-closed reads and exclusive concurrent creation. |
| src/praisonai-code/praisonai_code/cli/features/doctor/checks/bot_checks.py | Diagnostic guidance now explains automatic persistence and the multi-host override requirements. |
| src/praisonai/tests/unit/cli/test_interactive_surfaces_parity.py | The test double accepts the newly supported read-only execution argument. |
Flowchart
%%{init: {'theme': 'neutral'}}%%
flowchart TD
A[Pairing code or callback signing] --> B{Explicit override configured?}
B -->|Callback override| C[PRAISONAI_CALLBACK_SECRET]
B -->|Gateway override| D[PRAISONAI_GATEWAY_SECRET]
B -->|No override| E[Load persisted gateway secret]
E --> F{Secret file exists?}
F -->|Valid| G[Read and enforce owner-only permissions]
F -->|Missing| H[Create secret file exclusively with mode 0600]
C --> I[Sign or verify HMAC]
D --> I
G --> I
H --> I
Reviews (2): Last reviewed commit: "fix: harden gateway secret store and fix..." | Re-trigger Greptile
|
@claude You are the FINAL architecture reviewer. If the branch is under MervinPraison/PraisonAI (not a fork), you are able to make modifications to this branch and push directly. SCOPE: Focus ONLY on Python packages (praisonaiagents, praisonai). Do NOT modify praisonai-rust or praisonai-ts. Read ALL comments above from Gemini, Qodo, CodeRabbit, and Copilot carefully before responding. Phase 1: Review per AGENTS.md
Phase 2: FIX Valid Issues Phase 3: Final Verdict |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 4
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@src/praisonai-bot/praisonai_bot/bots/_pairing_ui.py`:
- Around line 28-35: Update _get_secret to remove the secrets.token_hex(32)
fallback and broad Exception suppression, allowing failures from
_load_or_create_secret and secret-store permission operations to propagate. If
documented storage failures must be handled, catch only those specific
exceptions, reuse one shared cached fallback, and emit an explicit warning so
the key remains consistent with gateway pairing codes.
In `@src/praisonai-bot/praisonai_bot/gateway/pairing.py`:
- Line 41: Update _load_or_create_secret to reject empty or whitespace-only
persisted content before decoding or using it. Only create and persist a new
secret when the path is missing; propagate read failures for existing paths
without overwriting them, ensuring pairing and callback signing never use an
empty key.
- Line 41: Update _load_or_create_secret to make initial .gateway_secret
creation atomic by using exclusive file creation (O_CREAT | O_EXCL) or an
equivalent lock. When another process wins the race, reread the existing secret
instead of generating or returning a different value, while preserving the
normal load behavior.
In `@src/praisonai-code/praisonai_code/cli/features/doctor/checks/bot_checks.py`:
- Around line 161-165: Update the gateway secret warning in the global_warnings
flow to document the PRAISONAI_CALLBACK_SECRET override: instruct users to unset
it or set it to the same value on every host so callback signatures align, while
retaining the existing PRAISONAI_GATEWAY_SECRET guidance.
🪄 Autofix
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro Plus
Run ID: 27d15325-3cea-44f1-bfa0-4398f7902f28
📒 Files selected for processing (3)
src/praisonai-bot/praisonai_bot/bots/_pairing_ui.pysrc/praisonai-bot/praisonai_bot/gateway/pairing.pysrc/praisonai-code/praisonai_code/cli/features/doctor/checks/bot_checks.py
| try: | ||
| from praisonai_bot.gateway.pairing import _DEFAULT_STORE_DIR, _load_or_create_secret | ||
|
|
||
| return _load_or_create_secret(_DEFAULT_STORE_DIR).decode() | ||
| except Exception: # pragma: no cover - defensive fallback | ||
| import secrets | ||
|
|
||
| return secrets.token_hex(32) |
There was a problem hiding this comment.
🗄️ Data Integrity & Integration | 🟠 Major | ⚡ Quick win
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo "== locate relevant files =="
fd -a '_pairing_ui.py|pairing.py' . || true
echo "== git diff stat =="
git diff --stat || true
echo "== inspect _pairing_ui.py around relevant section =="
if [ -f src/praisonai-bot/praisonai_bot/bots/_pairing_ui.py ]; then
wc -l src/praisonai-bot/praisonai_bot/bots/_pairing_ui.py
cat -n src/praisonai-bot/praisonai_bot/bots/_pairing_ui.py | sed -n '1,140p'
fi
echo "== locate shared secret symbols =="
python3 - <<'PY'
from pathlib import Path
root = Path('src/praisonai-bot')
for p in root.rglob('*.py'):
if (p.name == 'pairing.py' or p.name == '_pairing_ui.py') != 1:
# print pairs where at least one target exists currently
pass
for p in [root/'praisonai_bot'/gateway/'pairing.py', root/'praisonai_bot'/bots/'_pairing_ui.py']:
if p.exists():
print(f"-- {p} --")
lines = p.read_text().splitlines()
for i,l in enumerate(lines,1):
if 'secret' in l.lower() or '_DEFAULT_STORE_DIR' in l or '_load_or_create_secret' in l:
print(f"{i}: {l}")
PYRepository: MervinPraison/PraisonAI
Length of output: 6742
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo "== inspect gateway pairing.py secret symbols =="
wc -l src/praisonai-bot/praisonai_bot/gateway/pairing.py
cat -n src/praisonai-bot/praisonai_bot/gateway/pairing.py | sed -n '1,260p'
echo "== inspect bot pairing command secret symbols =="
wc -l src/praisonai-bot/praisonai_bot/cli/commands/pairing.py
cat -n src/praisonai-bot/praisonai_bot/cli/commands/pairing.py | sed -n '1,220p'
echo "== inspect callback verify usages =="
rg -n "_get_callback_secret|verify.*callback|hmac|callback_secret|PRAISONAI_CALLBACK_SECRET" src/praisonai-bot/praisonai_bot -SRepository: MervinPraison/PraisonAI
Length of output: 24308
Do not replace secret-store failures with an untracked random key.
_get_secret() and _load_or_create_secret() can fail while reading, saving, or signing secret permissions. This except Exception, secrets.token_hex(32) fallback lets those failures pass, and the random key can differ from the gateway secret already signed into pairing codes. Let unexpected failures propagate, or catch only documented storage/read/write failures with one shared cached fallback and an explicit warning.
🧰 Tools
🪛 Ruff (0.16.1)
[warning] 32-32: Do not catch blind exception: Exception
(BLE001)
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@src/praisonai-bot/praisonai_bot/bots/_pairing_ui.py` around lines 28 - 35,
Update _get_secret to remove the secrets.token_hex(32) fallback and broad
Exception suppression, allowing failures from _load_or_create_secret and
secret-store permission operations to propagate. If documented storage failures
must be handled, catch only those specific exceptions, reuse one shared cached
fallback, and emit an explicit warning so the key remains consistent with
gateway pairing codes.
Source: Linters/SAST tools
| use) so pairing codes survive process restarts with zero operator action. | ||
| """ | ||
| return os.environ.get("PRAISONAI_GATEWAY_SECRET", "") or secrets.token_hex(32) | ||
| return _load_or_create_secret(_DEFAULT_STORE_DIR).decode() |
There was a problem hiding this comment.
🔒 Security & Privacy | 🟠 Major | ⚡ Quick win
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo "Locate file:"
fd -a 'pairing.py$' . || true
echo
echo "Inspect pairing.py outline/imports and relevant functions:"
FILE="$(fd 'pairing.py$' . | head -n 1 || true)"
if [ -n "${FILE:-}" ]; then
wc -l "$FILE"
ast-grep outline "$FILE" || true
sed -n '1,140p' "$FILE"
fi
echo
echo "Search _load_or_create_secret and signing usages:"
rg -n "_load_or_create_secret|_DEFAULT_STORE_DIR|pairing|callback|sign|hmac|key" -S .Repository: MervinPraison/PraisonAI
Length of output: 50380
Fail closed on empty or unreadable persisted pairing state.
_load_or_create_secret accepts whitespace-only files after strip(), allowing an empty HMAC key. On read failures, create a new secret only for missing paths; do not overwrite an existing persisted secret. Reject empty content before creation so pairing/callback signatures cannot be generated or verified with an empty key.
[security_and_privACY]
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@src/praisonai-bot/praisonai_bot/gateway/pairing.py` at line 41, Update
_load_or_create_secret to reject empty or whitespace-only persisted content
before decoding or using it. Only create and persist a new secret when the path
is missing; propagate read failures for existing paths without overwriting them,
ensuring pairing and callback signing never use an empty key.
🗄️ Data Integrity & Integration | 🟠 Major | ⚡ Quick win
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo "Files matching pairing.py:"
fd -a 'pairing\.py$' . || true
echo
echo "Target file snippet:"
if [ -f src/praisonai-bot/praisonai_bot/gateway/pairing.py ]; then
nl -ba src/praisonai-bot/praisonai_bot/gateway/pairing.py | sed -n '1,140p'
else
echo "target file not found"
fi
echo
echo "Search secret resolver usage:"
rg -n "_load_or_create_secret|gateway_secret|secret" -S --glob '*.py' .Repository: MervinPraison/PraisonAI
Length of output: 463
🏁 Script executed:
#!/bin/bash
set -euo pipefail
python3 - <<'PY'
import os, tempfile, time
def both_like_current(path):
if not os.path.exists(path):
fd = os.open(path, os.O_CREAT|os.O_TRUNC, 0o600)
with os.fdopen(fd, 'w') as f:
f.write('secret\n')
time.sleep(0.1)
return open(path).read().rstrip()
return open(path).read().rstrip()
with tempfile.TemporaryDirectory() as d:
path = os.path.join(d, 'file')
# first checks, second checks, both open/write via os.open/trunc
t1 = os.dup(os.open(path, os.O_CREAT|os.O_TRUNC|os.O_EXCL, 0o600))
os.write(t1, b'winner\n')
os.close(t1)
# now show race with O_CREAT|O_TRUNC equivalent: second opens and truncates
t2 = os.dup(os.open(path, os.O_CREAT|os.O_TRUNC, 0o600))
os.write(t2, b'loser\n')
os.close(t2)
print(os.path.exists(path))
with open(path) as f:
print(f.read().rstrip())
# show O_EXCL behavior
path2 = os.path.join(d, 'unique')
fd = os.dup(os.open(path2, os.O_CREAT|os.O_TRUNC|os.O_EXCL, 0o600))
</PY>Repository: MervinPraison/PraisonAI
Length of output: 318
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo "Target file excerpt:"
awk '{printf "%5d %s\n", NR, $0}' src/praisonai-bot/praisonai_bot/gateway/pairing.py | sed -n '1,180p'
echo
echo "Related files excerpt:"
for f in src/praisonai-bot/praisonai_bot/cli/commands/pairing.py src/praisonai/praisonai/cli/commands/pairing.py; do
if [ -f "$f" ]; then
echo "--- $f"
awk '{printf "%5d %s\n", NR, $0}' "$f" | sed -n '1,180p'
fi
done
echo
echo "Secret resolver references:"
python3 - <<'PY'
import os, re
for root in ["."]:
for dirpath, _, filenames in os.walk(root):
if ".git" in dirpath or dirpath == "./.git":
continue
for name in filenames:
if not name.endswith(".py"):
continue
path = os.path.join(dirpath, name)
try:
text = open(path, "r", encoding="utf-8").read()
except Exception:
continue
if re.search(r"_load_or_create_secret|O_CREAT|O_TRUNC|O_EXCL|flock|fcntl\.lock|lockf|gateway_secret", text):
print(path)
for i, line in enumerate(text.splitlines(), 1):
if re.search(r"_load_or_create_secret|O_CREAT|O_TRUNC|O_EXCL|flock|fcntl\.lock|lockf|gateway_secret", line):
print(f"{i}: {line}")
PY
echo
echo "OS open race probe:"
python3 - <<'PY'
import os, tempfile, tempfile, threading, time
path = None
def waiter(path):
time.sleep(0.02)
fd = os.open(path, os.O_CREAT | os.O_TRUNC, 0o600)
os.write(fd, b"loser\n")
os.close(fd)
def opener(path):
time.sleep(0.02)
fd = os.open(path, os.O_CREAT | os.O_TRUNC, 0o600)
os.write(fd, b"winner\n")
os.close(fd)
with tempfile.TemporaryDirectory() as td:
path = os.path.join(td, "racefile")
t1 = threading.Thread(target=opener, args=(path,))
t2 = threading.Thread(target=waiter, args=(path,))
t1.start(); t2.start(); t1.join(); t2.join()
print(path, open(path).read().rstrip())
with tempfile.TemporaryDirectory() as td:
path = os.path.join(td, "exclfile")
fd = os.open(path, os.O_CREAT | os.O_EXCL | os.O_TRUNC, 0o600)
try:
fd2 = os.open(path, os.O_CREAT | os.O_EXCL | os.O_TRUNC, 0o600)
os.write(fd2, b"second\n")
os.close(fd2)
print("excl-second-open", open(path).read().rstrip())
except OSError as exc:
print("excl-second-open-fail", type(exc).__name__, exc.errno, open(path).read().rstrip())
PYRepository: MervinPraison/PraisonAI
Length of output: 28002
Make first-time secret creation atomic.
_load_or_create_secret() checks .gateway_secrets existence, then another process can create it before the current process opens it with O_CREAT | O_TRUNC. That path writes one secret but both processes can return different secrets, so one can issue pairing codes that fail under the receiver’s HMAC key. Use exclusive creation with O_CREAT | O_EXCL or a file lock, then reread the winner’s .gateway_secret.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@src/praisonai-bot/praisonai_bot/gateway/pairing.py` at line 41, Update
_load_or_create_secret to make initial .gateway_secret creation atomic by using
exclusive file creation (O_CREAT | O_EXCL) or an equivalent lock. When another
process wins the race, reread the existing secret instead of generating or
returning a different value, while preserving the normal load behavior.
| global_warnings.append( | ||
| "PRAISONAI_GATEWAY_SECRET not set - using auto-provisioned " | ||
| "persisted secret (~/.praisonai/gateway/.gateway_secret); set " | ||
| "the env-var to share one secret across hosts" | ||
| ) |
There was a problem hiding this comment.
🗄️ Data Integrity & Integration | 🟡 Minor | ⚡ Quick win
Document the callback override in cross-host guidance.
_get_callback_secret() uses PRAISONAI_CALLBACK_SECRET before PRAISONAI_GATEWAY_SECRET. Setting only the variable named here does not align callback signatures when PRAISONAI_CALLBACK_SECRET has a different value. State that the callback override must be unset or identical on every host.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@src/praisonai-code/praisonai_code/cli/features/doctor/checks/bot_checks.py`
around lines 161 - 165, Update the gateway secret warning in the global_warnings
flow to document the PRAISONAI_CALLBACK_SECRET override: instruct users to unset
it or set it to the same value on every host so callback signatures align, while
retaining the existing PRAISONAI_GATEWAY_SECRET guidance.
|
@claude CI failed on HEAD Failed checks
Failures (extracted)
Critical review firstBefore changing code or tests, decide which side is wrong:
SDK guardrails (AGENTS.md):
What to do
|
|
Claude finished @MervinPraison's task —— View job Final Architecture Review — PR #3781 ✅Verdict: APPROVE (after fixes pushed to this branch).
Phase 1 — Architecture review
Phase 2 — CI failure (root cause) + reviewer fixesCI Valid CodeRabbit findings — fixed:
Files modified
Skipped: nothing material — all 4 reviewer findings addressed. Phase 3 — Validation
All issues resolved. Ready to merge. Job run • Branch |
- pairing.py: reject empty/whitespace secrets, fail closed on read errors of an existing secret (no silent HMAC rotation), and create the secret atomically with O_CREAT|O_EXCL so racing processes converge on one key. - _pairing_ui.py: narrow the callback-secret fallback to storage errors and warn explicitly instead of silently substituting a diverging random key. - bot_checks.py: doctor guidance now notes PRAISONAI_CALLBACK_SECRET must be unset/identical across hosts for button signatures to align. - test_interactive_surfaces_parity.py: update the _execute_prompt fake to accept read_only (matching the real signature added in #3758), fixing KeyError:'prompt'. 🤖 Generated with [Claude Code](https://claude.ai/code) Co-authored-by: Mervin Praison <MervinPraison@users.noreply.github.com>
MervinPraison
added
pipeline/blocked:stale-final
|
Claude finished @MervinPraison's task —— View job CI Fix — PR #3781Verdict: pre-existing merge collision, already fixed on current HEAD — no code changes needed. The CI report targeted the stale HEAD
Root cause of
|
| # | File | Fix |
|---|---|---|
| 🟠 | _pairing_ui.py |
Narrowed broad except Exception → (OSError, IOError); single cached fallback via lru_cache; explicit warning (no silent key rotation) |
| 🟠 | pairing.py |
Rejects empty/whitespace-only secret; fails closed on read failure of an existing secret |
| 🟠 | pairing.py |
Atomic first-time creation via O_CREAT|O_EXCL; race loser re-reads winner's secret |
| 🟡 | bot_checks.py |
Doctor guidance now notes PRAISONAI_CALLBACK_SECRET must be unset/identical across hosts |
Architecture (AGENTS.md)
Wrapper-only (praisonai-bot, praisonai-code). No praisonaiagents/ core changes, no new Agent params, no new deps. Env overrides preserved → backward compatible. ✅
Files modified this branch (vs fa702ec2): gateway/pairing.py, bots/_pairing_ui.py, doctor/checks/bot_checks.py, tests/unit/cli/test_interactive_surfaces_parity.py. Skipped: nothing material.
Working tree clean; all fixes are on remote HEAD ef9719418. Ready to merge once CI re-runs.
praisonai-triage-agent
Bot
removed
the
pipeline/blocked:cooldown
1 participant
Fixes #3779
Summary
The gateway signs pairing codes and interactive approve/deny buttons with HMAC secrets. The callback-button path (
bots/_pairing_ui.py) defaulted to a per-process random secret under a different env var (PRAISONAI_CALLBACK_SECRET), so any pending Approve/Deny button issued before a restart silently failed signature verification afterwards.This PR unifies both signing helpers onto the single persisted, auto-provisioned secret that the pairing-code path already used (
~/.praisonai/gateway/.gateway_secret, created0600), so buttons and codes keep working across restarts with zero operator action. An explicit env override still wins.Changes
bots/_pairing_ui.py:_get_callback_secret()now resolves through the persisted gateway secret store (_load_or_create_secret).PRAISONAI_CALLBACK_SECREToverride still takes precedence; the persisted read is cached.gateway/pairing.py: collapsed the stale ephemeral_get_secret()onto the persisted store and fixed its docstring (it previously claimed codes would not survive restarts).doctor/checks/bot_checks.py: corrected the security-check message — the secret now persists by default; the env-var is only needed to share one secret across hosts.Layer placement
Wrapper only (gateway operational state). No core SDK, no new params, no new dependencies.
Validation
0600); explicit env override wins;_get_secret()stable across calls.Generated with Claude Code
Summary by CodeRabbit
Bug Fixes
Documentation