Issue precise subscription tokens without revocation bypass - #758
Issue precise subscription tokens without revocation bypass#758Rerowros wants to merge 2 commits into
Conversation
|
Important Review skippedAuto reviews are disabled on base/target branches other than the default branch. Please check the settings in the CodeRabbit UI or the ⚙️ Run configurationConfiguration used: Organization UI 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:
WalkthroughThe pull request adds multi-worker NATS coordination, shared node lifecycle state, administrator-scoped group validation, precise v4 subscription tokens, optimized WireGuard reconciliation, subscription variable formatting, dashboard updates, and workflow maintenance. ChangesPlatform coordination and shared worker state
Application behavior and access contracts
Estimated code review effort: 5 (Critical) | ~90 minutes Possibly related PRs
Suggested labels: Poem
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✨ Finishing Touches🧪 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 |
There was a problem hiding this comment.
Pull request overview
This PR updates subscription token issuance/parsing and revocation validation to avoid “future-dated” timestamps and to ensure revocation checks fail closed at microsecond boundaries, with targeted regression tests to lock in boundary behavior.
Changes:
- Switch subscription token issuance to a v4 format storing epoch nanoseconds (microsecond-accurate in payload decoding).
- Preserve token version during parsing and tighten revocation comparisons (including conservative handling for older ceil-rounded tokens).
- Add regression tests covering v3 future-rounding behavior and v4 revocation tie behavior, plus deterministic issuance-time coverage.
Reviewed changes
Copilot reviewed 4 out of 4 changed files in this pull request and generated 2 comments.
| File | Description |
|---|---|
app/utils/jwt.py |
Issues v4 tokens with time_ns() and parses v2/v3/v4/legacy payloads, including token version propagation. |
app/operation/__init__.py |
Updates subscription validation to apply version-aware revocation comparisons and fail-closed semantics. |
tests/api/test_user.py |
Adds deterministic test asserting v4 issuance time is precise and non-future-dated. |
tests/test_subscription_token_revocation.py |
Adds revocation-boundary regression tests for v3 future-rounding and v4 tie-at-microsecond behavior. |
💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.
a3b2d43 to
7aaf331
Compare
Rerowros
changed the title
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 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 `@tests/test_subscription_token_revocation.py`:
- Around line 22-38: Extend the subscription validation tests around
get_validated_sub to cover a timestamp-only payload without token_version, with
token created_at equal to sub_revoked_at, and assert rejection. Update the
validator’s legacy branch in get_validated_sub so legacy tokens use an inclusive
revocation comparison, while preserving strict > semantics for v2 and v3 tokens.
🪄 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: Organization UI
Review profile: CHILL
Plan: Pro Plus
Run ID: 71879964-8156-4ad9-8ca5-6f6ac2c2f959
📒 Files selected for processing (2)
app/operation/__init__.pytests/test_subscription_token_revocation.py
🚧 Files skipped from review as they are similar to previous changes (1)
- app/operation/init.py
b36be99 to
2825c7f
Compare
There was a problem hiding this comment.
Actionable comments posted: 14
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (3)
dashboard/src/pages/_dashboard.settings.general.tsx (1)
54-58: 🗄️ Data Integrity & Integration | 🟠 Major | ⚡ Quick winKeep
subscription.custom_variableswhen saving general settings.
modify_settingsreplacesgeneralwhenfilteredData.generalis sent. This payload does not includegeneral.custom_variablesor the full existing settings, so existingsettings.subscription.custom_variablesare dropped from the JSON row. Send subscriptioncustom_variablesin the payload or makegenerala partial update.🤖 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 `@dashboard/src/pages/_dashboard.settings.general.tsx` around lines 54 - 58, Update the filteredData payload in the general settings save flow to preserve the existing subscription.custom_variables under general.custom_variables, either by including them in the payload or by using a partial general-settings update. Ensure modify_settings does not replace general without carrying forward these existing custom variables..github/workflows/test-database-migrations.yml (1)
16-17: 🔒 Security & Privacy | 🟡 Minor | ⚡ Quick winAdd an explicit least-privilege
permissionsblock.The workflow declares no
permissions, so jobs receive the repository default token scopes. These jobs only check out code and run migrations and tests. Set read-only permissions at the workflow level.🔒 Proposed fix
+permissions: + contents: read + jobs: test-sqlite:🤖 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 @.github/workflows/test-database-migrations.yml around lines 16 - 17, Add a workflow-level permissions block near the top-level configuration of the migration workflow, granting only read access to repository contents. Keep the existing test-sqlite job behavior unchanged while ensuring checkout and test steps operate with least-privilege token permissions.Source: Linters/SAST tools
app/node/__init__.py (1)
60-74: 🩺 Stability & Availability | 🟠 Major | ⚡ Quick winDo not call
node.stop()from siblingupsert/connectsync.
_update_node_syncpublishesupsert, andhandle_node_messagecallsnode_manager.update_node(db_node)on sibling workers beforeremove/disconnectuseremote_stop=False. Becauseupdate_nodeuses_shutdown_node(old_node)withremote_stop=True, each sibling running local sync can also shut the same remote core while other workers still use it. Addremote_stop=Falsetoupdate_nodefor the shared-sync path or skip remote stops when bridge memory is active.🤖 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 `@app/node/__init__.py` around lines 60 - 74, The update_node flow currently calls _shutdown_node(old_node) with remote stopping enabled, causing sibling upsert/connect synchronization to stop a shared remote core. Change update_node’s shutdown behavior for the shared-sync path to use remote_stop=False, or skip remote stops when bridge memory is active, while preserving local cleanup and the existing remove/disconnect remote-stop behavior.
🧹 Nitpick comments (5)
tests/api/test_user.py (1)
187-203: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valuePatch the module reference instead of the stdlib
timemodule.Line 195 sets
time_nson the sharedtimemodule object. Every caller in the process observes the frozen value for the duration of the test. Patch a module-local seam instead.The timestamp assertion itself is correct:
1_723_000_000seconds is2024-08-07T03:06:40Z, and the sub-second part truncates to123456microseconds.♻️ Proposed change
+ from types import SimpleNamespace + monkeypatch.setattr(jwt_utils, "get_secret_key", fake_get_secret_key) - monkeypatch.setattr(jwt_utils.time, "time_ns", lambda: issued_at_ns) + monkeypatch.setattr(jwt_utils, "time", SimpleNamespace(time_ns=lambda: issued_at_ns))🤖 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 `@tests/api/test_user.py` around lines 187 - 203, Update test_subscription_token_uses_precise_non_future_issuance_time to patch the module-local time_ns seam used by the token implementation, rather than mutating the shared jwt_utils.time module object. Keep the issued_at_ns value and timestamp assertion unchanged.app/jobs/node_checker.py (1)
255-262: 🚀 Performance & Scalability | 🔵 Trivial | ⚡ Quick winAdd startup jitter so worker health loops do not run in phase.
Every uvicorn worker starts
_interval_loopat process start and uses the samecore_health_check_interval. The workers therefore runnode_health_checkat nearly the same instant on every tick. Each run queries all nodes from the database and contacts every node. With several workers this produces a synchronized load spike against the database and the node API on each interval.Add a small random initial delay before the first iteration.
♻️ Proposed refactor
async def _interval_loop(coro, seconds: float, name: str): """Run node maintenance on every worker (APScheduler may be leader-only).""" + # Stagger workers so health ticks do not align across processes. + await asyncio.sleep(random.uniform(0, min(seconds, 5))) while True: try: await coro() except Exception as exc: logger.error("Node loop %s failed: %s", name, exc) await asyncio.sleep(seconds)Add
import randomat the top of the file.🤖 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 `@app/jobs/node_checker.py` around lines 255 - 262, Update _interval_loop to await a small random delay before entering its first maintenance iteration, using the module’s random import as suggested. Keep the existing repeated coro execution, error logging, and interval sleep behavior unchanged after startup.app/operation/node.py (2)
233-258: 🩺 Stability & Availability | 🔵 Trivial | ⚡ Quick winRaise the log level when attach fails for a non-routine reason.
Line 257 logs every attach failure at
debug. The handler catches all exceptions, so a KV outage, an authentication failure, or a bridge bug is indistinguishable from the routine "core is not running" case. The caller then falls back tostart(), which hides the fault. Log atwarningfor unexpected exception types, or include the exception type in the message so operators can filter it.🤖 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 `@app/operation/node.py` around lines 233 - 258, Update _attach_if_running’s broad exception handler so unexpected failures such as KV, authentication, or bridge errors are logged at warning level or otherwise include the exception type, while preserving debug-level logging for routine attach-skipped conditions. Keep the fallback return None behavior unchanged.
260-277: 🚀 Performance & Scalability | 🔵 Trivial | ⚡ Quick winAvoid the duplicate lifecycle-state read on the connect path.
Line 262 calls
pg_node.get_lifecycle_state()._attach_if_runningthen callsget_lifecycle_state()again at line 237. Each call is a NATS KV read in shared-bridge mode. Bulk connect runs up toCONNECT_CONCURRENCYnodes in parallel, so this doubles lifecycle KV traffic during startup, which is the exact contention the concurrency cap targets.Pass the already-fetched state into
_attach_if_runningas an optional argument.♻️ Proposed refactor
`@staticmethod` - async def _attach_if_running(pg_node: PasarGuardNode, node_name: str): + async def _attach_if_running(pg_node: PasarGuardNode, node_name: str, state=None): """Attach to an already-started remote core without calling Start RPC.""" try: - state = await pg_node.get_lifecycle_state() + if state is None: + state = await pg_node.get_lifecycle_state()state = await pg_node.get_lifecycle_state() if state is not None and state.observed is LifecycleStatus.HEALTHY: - attached = await NodeOperation._attach_if_running(pg_node, db_node.name) + attached = await NodeOperation._attach_if_running(pg_node, db_node.name, state) if attached is not None: return attached🤖 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 `@app/operation/node.py` around lines 260 - 277, Update _start_or_attach_node and _attach_if_running to accept an optional pre-fetched lifecycle state, pass the state read by _start_or_attach_node into _attach_if_running, and have _attach_if_running reuse it instead of calling get_lifecycle_state again; retain its existing read behavior when no state is supplied.app/nats/kv_cas.py (1)
61-66: 🚀 Performance & Scalability | 🔵 Trivial | ⚡ Quick winPush the prefix into
kv.keys()instead of filtering in Python.
kv_list_keysfetches every key in the bucket, then filters by prefix. The user-sync bucket holds pending and claimed keys for all nodes.NatsUserSyncStore.claim_userscalls this twice per claim cycle, so the cost grows with total bucket size rather than with the keys for one node.The
CasKvprotocol already declares afiltersparameter, and NATS KV supports subject filters. Pass the prefix through.♻️ Proposed change
async def kv_list_keys(kv: CasKv, prefix: str) -> list[str]: try: - keys = await kv.keys() + keys = await kv.keys(filters=[f"{prefix}>"]) except nats_js_errors.NoKeysError: return [] return [key for key in keys if key.startswith(prefix)]
MemoryCasKv.keysuses substring matching, so update it to match NATS subject-filter semantics if you adopt this.Confirm the exact
filterssemantics forKeyValue.keysin nats-py 2.15.0 before applying.🤖 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 `@app/nats/kv_cas.py` around lines 61 - 66, Update kv_list_keys to pass prefix as the filters argument to kv.keys() and remove the Python-side filtering, preserving the NoKeysError empty-list behavior. Verify nats-py 2.15.0 KeyValue.keys filter semantics first, then update MemoryCasKv.keys to match NATS subject-filter semantics rather than substring matching if needed.
🤖 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 `@app/app_factory.py`:
- Around line 149-157: Update _pause_jobs_on_leadership_lost to pause the
scheduler before awaiting stop_notification_dispatcher; move the
scheduler.running check and scheduler.pause() ahead of the notification shutdown
while preserving the existing notification state reset and reclaim-task
behavior.
In `@app/db/crud/wireguard.py`:
- Around line 320-362: Bound WireGuard reconciliation memory by replacing
all-result materialization in app/db/crud/wireguard.py:320-362 with page-scoped
tag and proxy-settings retrieval, preserving stable user ordering and chunk
limits. Update the reconciliation flow at app/db/crud/wireguard.py:541-546 to
select users in stable keyset pages, process each page completely before
fetching the next, and avoid accumulating accessible tags, peer-IP IDs, proxy
settings, desired values, or allocation candidates across pages.
In `@app/jobs/node_checker.py`:
- Around line 315-320: Align the shutdown policy by removing the workers-count
restriction from the on_shutdown(shutdown_nodes) registration near the
feature_settings.stop_nodes_on_shutdown check. Let shutdown_nodes own the
multi-worker decision through its existing is_multi_worker() and
server_settings.workers guard, preserving that guard for direct calls.
- Around line 174-175: Update both update_observed_lifecycle calls in the
node-checking flow to capture their return values and detect rejected CAS
updates caused by an epoch mismatch. Log each failed update with sufficient
context instead of awaiting the calls without inspecting the result, while
preserving the existing lifecycle status and expected_epoch arguments.
In `@app/nats/leader.py`:
- Around line 253-256: Guard the NATS connection close in stop_job_leader at
app/nats/leader.py#L253-L256 with contextlib.suppress(Exception), preserving the
unconditional _nc and _kv resets. Apply the same change in
shutdown_bridge_memory at app/node/nats_memory.py#L436-L444, ensuring all five
global resets execute even when close() fails; contextlib is already imported in
both files.
In `@app/node/__init__.py`:
- Around line 26-47: Update _create_node_kwargs to remove the unsupported
node_id, user_sync_store, lifecycle_coordinator, and worker_id entries from the
create_node keyword arguments. Keep the standard node client fields and extra
metadata, and remove the now-unneeded get_bridge_memory call and conditional
block.
In `@app/operation/node.py`:
- Around line 704-709: Update _connect_nodes_bulk_local to return its
valid_results, then revise _connect_nodes_bulk_sync to publish connect events
only for results with NodeStatus.connected, using the result’s node ID. Replace
the sequential eligible-node publishing loop with asyncio.gather while
preserving the existing disabled/limited filtering and avoiding publishes for
failed connections.
In `@app/operation/subscription.py`:
- Around line 274-283: Update create_info_response_headers to build format
variables through get_format_variables, matching subscription response
formatting so url and TEMPLATE_TITLE resolve consistently in announce and
announce-url. Preserve the existing custom-variable application and formatting
flow after using the shared builder.
In `@app/subscription/share.py`:
- Line 312: Update the SNI handling around the sni.format_map call to prevent
malformed templates such as unmatched braces from aborting subscription
generation. Prefer validating SNI templates when they are saved; otherwise catch
formatting failures and safely retain the original raw SNI value while
preserving the existing empty-string behavior.
In
`@dashboard/src/features/subscriptions/components/subscription-general-settings-section.tsx`:
- Around line 139-140: Add a translated aria-label to the icon-only trigger
rendered by VariablesPopover, using the existing translation mechanism and an
appropriate variable-related label. Update VariablesPopover rather than
CustomVariablesPopover, while preserving the current trigger behavior.
In
`@dashboard/src/features/subscriptions/components/subscription-settings-schema.ts`:
- Line 105: Validate the URL produced by _format_announce_url after variable
expansion, before emitting announce-url, while preserving the empty-value
allowance. Reject invalid expanded URLs and fall back safely when custom
variables in the stored value cannot be resolved. Add coverage for empty,
successfully token-expanded, and invalid expanded URLs.
In `@pyproject.toml`:
- Around line 40-41: Update the pasarguard-node-bridge dependency declaration in
pyproject.toml to require the latest available compatible public PyPI release,
0.8.1, or otherwise align it with a published 0.9.0 registry release; ensure the
lockfile dependency resolution remains consistent.
In `@tests/api/test_core.py`:
- Around line 60-96: Update
test_wireguard_core_create_skips_user_scan_and_allocates_on_group to spy on
CoreOperation._reconcile_wireguard during the create_core call and assert it is
not invoked. Keep the existing peer_ips assertion, but make the test directly
distinguish pool initialization from the former reconciliation path.
In `@tests/test_nats_leader_steal.py`:
- Around line 10-30: Add an autouse fixture in the test module that resets the
leader module globals before and after each test, matching the existing pattern
in test_nats_leader_heartbeat.py. Ensure _is_leader and _token are restored to
their inactive values so tests involving try_become_leader do not leak singleton
state.
---
Outside diff comments:
In @.github/workflows/test-database-migrations.yml:
- Around line 16-17: Add a workflow-level permissions block near the top-level
configuration of the migration workflow, granting only read access to repository
contents. Keep the existing test-sqlite job behavior unchanged while ensuring
checkout and test steps operate with least-privilege token permissions.
In `@app/node/__init__.py`:
- Around line 60-74: The update_node flow currently calls
_shutdown_node(old_node) with remote stopping enabled, causing sibling
upsert/connect synchronization to stop a shared remote core. Change
update_node’s shutdown behavior for the shared-sync path to use
remote_stop=False, or skip remote stops when bridge memory is active, while
preserving local cleanup and the existing remove/disconnect remote-stop
behavior.
In `@dashboard/src/pages/_dashboard.settings.general.tsx`:
- Around line 54-58: Update the filteredData payload in the general settings
save flow to preserve the existing subscription.custom_variables under
general.custom_variables, either by including them in the payload or by using a
partial general-settings update. Ensure modify_settings does not replace general
without carrying forward these existing custom variables.
---
Nitpick comments:
In `@app/jobs/node_checker.py`:
- Around line 255-262: Update _interval_loop to await a small random delay
before entering its first maintenance iteration, using the module’s random
import as suggested. Keep the existing repeated coro execution, error logging,
and interval sleep behavior unchanged after startup.
In `@app/nats/kv_cas.py`:
- Around line 61-66: Update kv_list_keys to pass prefix as the filters argument
to kv.keys() and remove the Python-side filtering, preserving the NoKeysError
empty-list behavior. Verify nats-py 2.15.0 KeyValue.keys filter semantics first,
then update MemoryCasKv.keys to match NATS subject-filter semantics rather than
substring matching if needed.
In `@app/operation/node.py`:
- Around line 233-258: Update _attach_if_running’s broad exception handler so
unexpected failures such as KV, authentication, or bridge errors are logged at
warning level or otherwise include the exception type, while preserving
debug-level logging for routine attach-skipped conditions. Keep the fallback
return None behavior unchanged.
- Around line 260-277: Update _start_or_attach_node and _attach_if_running to
accept an optional pre-fetched lifecycle state, pass the state read by
_start_or_attach_node into _attach_if_running, and have _attach_if_running reuse
it instead of calling get_lifecycle_state again; retain its existing read
behavior when no state is supplied.
In `@tests/api/test_user.py`:
- Around line 187-203: Update
test_subscription_token_uses_precise_non_future_issuance_time to patch the
module-local time_ns seam used by the token implementation, rather than mutating
the shared jwt_utils.time module object. Keep the issued_at_ns value and
timestamp assertion unchanged.
🪄 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: Organization UI
Review profile: CHILL
Plan: Pro Plus
Run ID: da72b71d-bd8d-4d9d-9a07-107c2adf8e1c
⛔ Files ignored due to path filters (1)
uv.lockis excluded by!**/*.lock
📒 Files selected for processing (52)
.env.example.github/workflows/api-codeql.yml.github/workflows/build-dev.yml.github/workflows/build.yml.github/workflows/frontend-codeql.yml.github/workflows/test-database-migrations.ymlapp/app_factory.pyapp/core/hosts.pyapp/core/manager.pyapp/db/crud/wireguard.pyapp/jobs/node_checker.pyapp/jobs/record_usages.pyapp/nats/__init__.pyapp/nats/kv_cas.pyapp/nats/leader.pyapp/nats/message.pyapp/nats/router.pyapp/node/__init__.pyapp/node/manager_sync.pyapp/node/nats_memory.pyapp/operation/__init__.pyapp/operation/core.pyapp/operation/group.pyapp/operation/node.pyapp/operation/subscription.pyapp/operation/user.pyapp/operation/user_template.pyapp/routers/group.pyapp/subscription/share.pyconfig.pydashboard/public/statics/locales/en.jsondashboard/public/statics/locales/fa.jsondashboard/public/statics/locales/ru.jsondashboard/public/statics/locales/zh.jsondashboard/src/components/ui/variables-popover.tsxdashboard/src/features/hosts/dialogs/host-modal.tsxdashboard/src/features/subscriptions/components/subscription-general-settings-section.tsxdashboard/src/features/subscriptions/components/subscription-settings-schema.tsdashboard/src/features/users/components/action-buttons.tsxdashboard/src/pages/_dashboard.settings.general.tsxpyproject.tomlrole.pytests/api/test_core.pytests/api/test_user.pytests/test_connect_concurrency.pytests/test_create_app_nats_guard.pytests/test_group_access_unit.pytests/test_nats_leader_heartbeat.pytests/test_nats_leader_steal.pytests/test_nats_node_memory.pytests/test_node_manager_sync.pytests/test_subscription_token_revocation.py
There was a problem hiding this comment.
Caution
Inline review comments failed to post. This is likely due to GitHub's internal server error or limits when posting large numbers of comments. If you are seeing this consistently it is likely a permissions issue. Please check "Moderation" -> "Code review limits" under your organization settings.
🛑 Comments failed to post (14)
app/app_factory.py (1)
149-157: 🗄️ Data Integrity & Integration | 🟠 Major | ⚡ Quick win
Pause APScheduler before awaiting notification shutdown.
Line 151 can yield while this worker is no longer the leader. The scheduler remains active until Line 156. A scheduled job can run during that interval and duplicate leader-only work, including node limit updates.
Proposed fix
async def _pause_jobs_on_leadership_lost(): + if scheduler.running: + scheduler.pause() if started_notifications["value"]: from app.notification.client import stop_notification_dispatcher await stop_notification_dispatcher() started_notifications["value"] = False - if scheduler.running: - scheduler.pause() _ensure_reclaim_task()📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.async def _pause_jobs_on_leadership_lost(): if scheduler.running: scheduler.pause() if started_notifications["value"]: from app.notification.client import stop_notification_dispatcher await stop_notification_dispatcher() started_notifications["value"] = False _ensure_reclaim_task()🤖 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 `@app/app_factory.py` around lines 149 - 157, Update _pause_jobs_on_leadership_lost to pause the scheduler before awaiting stop_notification_dispatcher; move the scheduler.running check and scheduler.pause() ahead of the notification shutdown while preserving the existing notification state reset and reclaim-task behavior.app/db/crud/wireguard.py (1)
320-362: 🚀 Performance & Scalability | 🟠 Major | 🏗️ Heavy lift
Process WireGuard reconciliation as bounded pages.
The new chunk size limits individual SQL
INlists. It does not bound total memory. Reconciliation still materializes all accessible-tag rows, peer-IP IDs, proxy settings, desired peer-IP values, and allocation candidates before updates start.
app/db/crud/wireguard.py#L320-L362: replace complete list and dictionary results with page-scoped tag and settings retrieval.app/db/crud/wireguard.py#L541-L546: select relevant users in stable keyset pages and process each page before loading the next one.📍 Affects 1 file
app/db/crud/wireguard.py#L320-L362(this comment)app/db/crud/wireguard.py#L541-L546🤖 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 `@app/db/crud/wireguard.py` around lines 320 - 362, Bound WireGuard reconciliation memory by replacing all-result materialization in app/db/crud/wireguard.py:320-362 with page-scoped tag and proxy-settings retrieval, preserving stable user ordering and chunk limits. Update the reconciliation flow at app/db/crud/wireguard.py:541-546 to select users in stable keyset pages, process each page completely before fetching the next, and avoid accumulating accessible tags, peer-IP IDs, proxy settings, desired values, or allocation candidates across pages.app/jobs/node_checker.py (2)
174-175: 🩺 Stability & Availability | 🟡 Minor | ⚡ Quick win
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash python - <<'PY' import inspect try: from PasarGuardNodeBridge import PasarGuardNode print(inspect.signature(PasarGuardNode.update_observed_lifecycle)) print(inspect.getsource(PasarGuardNode.update_observed_lifecycle)) except Exception as exc: print("inspect failed:", exc) PY # Show every call site so the handling stays consistent. rg -nP -C3 'update_observed_lifecycle\s*\(' --type=pyRepository: PasarGuard/panel
Length of output: 209
🏁 Script executed:
#!/bin/bash set -euo pipefail echo "== files ==" git ls-files | sed -n '1,120p' echo "== locate node_checker ==" fd -a 'node_checker.py' . || true echo "== outline ==" ast-grep outline app/jobs/node_checker.py || true echo "== relevant lines ==" sed -n '120,215p' app/jobs/node_checker.py echo "== search shared_state/update calls ==" rg -n -C 4 'shared_state|update_observed_lifecycle|get_lifecycle_state|LifecycleStatus\.BROKEN' app/jobs/node_checker.py app || true echo "== lifecycle symbols references ==" rg -n 'class PasarGuardNode|def update_observed_lifecycle|def get_lifecycle_state|observed_lifecycle|LifecycleStatus' . --type=py --glob '!**/__pycache__/**' || trueRepository: PasarGuard/panel
Length of output: 21032
🏁 Script executed:
#!/bin/bash set -euo pipefail echo "== nats_memory lifecycle state ==" sed -n '320,390p' app/node/nats_memory.py echo "== node_checker lifecycle imports/context ==" sed -n '1,90p' app/jobs/node_checker.py echo "== lifecycle tests ==" sed -n '1,180p' tests/test_nats_node_memory.py echo "== deterministic CAS-equivalent behavioral probe from source text ==" python3 - <<'PY' from pathlib import Path p = Path("app/node/nats_memory.py").read_text() start = p.index(" async def update_observed(") end = p.index(" async def update_shared_state", start) method = p[start:end] print(method) print("\nexpected_epoch appears:", "expected_epoch" in method) print("returns None:", "return None" in method or method.strip().endswith("None")) print("raises exception on stale epoch:", "raise " in method) PYRepository: PasarGuard/panel
Length of output: 12756
Log rejected observed-lifecycle Cas updates.
update_observed_lifecycle(..., expected_epoch=shared_state.epoch)can be discarded whenexpected_epochno longer matches, and the next shared-state writer will then see an older observed status. Capture and log the failed return for both calls instead of usingawait node.update_observed_lifecycle(...)as a fire-and-forget health-side note.🤖 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 `@app/jobs/node_checker.py` around lines 174 - 175, Update both update_observed_lifecycle calls in the node-checking flow to capture their return values and detect rejected CAS updates caused by an epoch mismatch. Log each failed update with sufficient context instead of awaiting the calls without inspecting the result, while preserving the existing lifecycle status and expected_epoch arguments.
315-320: 📐 Maintainability & Code Quality | 🟡 Minor | ⚡ Quick win
Align the two shutdown guards; they express different policies.
Line 316 registers
shutdown_nodesonly whenserver_settings.workers <= 1. Line 334 insideshutdown_nodesskips the stop whenis_multi_worker() and server_settings.workers > 1.The conditions differ. If an operator sets
UVICORN_WORKERS=2withoutNATS_ENABLED=1, thenis_multi_worker()is False butworkers > 1. The registration at line 316 skipsshutdown_nodesentirely, so remote cores are never stopped, while the runtime guard at line 334 would have permitted the stop. The registration decides one way and the function body the other.Use the same predicate in both places. Because registration already filters, the check at line 334 is unreachable through this path and only matters for direct calls.
♻️ Proposed fix
- # Multi-uvicorn workers must not Stop remote cores / clear shared sync queues on exit. - if feature_settings.stop_nodes_on_shutdown and server_settings.workers <= 1: + # Multi-uvicorn workers must not Stop remote cores / clear shared sync queues on exit. + if feature_settings.stop_nodes_on_shutdown: on_shutdown(shutdown_nodes)
shutdown_nodesthen owns the multi-worker decision through its existing guard at line 334.Also applies to: 331-336
🤖 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 `@app/jobs/node_checker.py` around lines 315 - 320, Align the shutdown policy by removing the workers-count restriction from the on_shutdown(shutdown_nodes) registration near the feature_settings.stop_nodes_on_shutdown check. Let shutdown_nodes own the multi-worker decision through its existing is_multi_worker() and server_settings.workers guard, preserving that guard for direct calls.app/nats/leader.py (1)
253-256: 🩺 Stability & Availability | 🟡 Minor | ⚡ Quick win
Unguarded
await _nc.close()in two shutdown paths. Both functions close the NATS connection without suppressing exceptions, and both reset module globals only on the lines that follow. If the connection is already broken,close()raises, the reset lines never run, and the shutdown hook chain aborts. Stale globals then make a later re-initialization in the same process reuse a dead client.ensure_bridge_memoryinapp/node/nats_memory.pyalready wraps a comparable close withcontextlib.suppress; apply the same pattern to both sites.
app/nats/leader.py#L253-L256: wrapawait _nc.close()instop_job_leaderwithcontextlib.suppress(Exception)so_nc = Noneand_kv = Nonealways run.contextlibis imported at line 6.app/node/nats_memory.py#L436-L444: wrapawait _nc.close()inshutdown_bridge_memorywithcontextlib.suppress(Exception)so the five global resets always run.contextlibis imported at line 7.📍 Affects 2 files
app/nats/leader.py#L253-L256(this comment)app/node/nats_memory.py#L436-L444🤖 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 `@app/nats/leader.py` around lines 253 - 256, Guard the NATS connection close in stop_job_leader at app/nats/leader.py#L253-L256 with contextlib.suppress(Exception), preserving the unconditional _nc and _kv resets. Apply the same change in shutdown_bridge_memory at app/node/nats_memory.py#L436-L444, ensuring all five global resets execute even when close() fails; contextlib is already imported in both files.app/node/__init__.py (1)
26-47: 🩺 Stability & Availability | 🔴 Critical | ⚡ Quick win
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash # Locate the installed bridge package and inspect create_node's signature. fd -t d 'PasarGuardNodeBridge' / 2>/dev/null | head python - <<'PY' import inspect try: import PasarGuardNodeBridge as b print(inspect.signature(b.create_node)) except Exception as exc: print("import failed:", exc) PY # Fallback: check the declared dependency version in the manifest. rg -n 'pasarguard[-_]node[-_]bridge' --iglob 'pyproject.toml' --iglob 'requirements*.txt' --iglob 'uv.lock'Repository: PasarGuard/panel
Length of output: 208
🌐 Web query:
pasarguard-node-bridge 0.9.0 create_node signature node_id user_sync_store lifecycle_coordinator worker_id💡 Result:
The pasarguard-node-bridge library does not support a create_node method with the signature containing node_id, user_sync_store, lifecycle_coordinator, and worker_id. Official documentation and repository examples for pasarguard-node-bridge show that the create_node method is used to instantiate a node client for connecting to a PasarGuard node, accepting parameters such as connection, address, port, api_port, server_ca, api_key, name, extra, default_timeout, internal_timeout, and proxy [1][2][3]. The parameters you mentioned—node_id, user_sync_store, lifecycle_coordinator, and worker_id—do not appear in the library's create_node API, nor are they mentioned in the associated documentation for this specific Python package [1][4][5]. It is possible these terms refer to an internal component architecture or a different library entirely.
Citations:
- 1: https://pypi.org/project/pasarguard-node-bridge/
- 2: https://github.com/PasarGuard/node_bridge_py
- 3: https://github.com/pasarguard/node_bridge_py
- 4: https://pypi.org/project/pasarguard-node-bridge/0.8.0/
- 5: https://context7.com/pasarguard/node_bridge_py/llms.txt
🌐 Web query:
pasarguard-node-bridge 0.9.0 create_node API documentation💡 Result:
The pasarguard-node-bridge library is a Python package (often referred to as node_bridge_py) used to interact with PasarGuard nodes [1][2]. As of the available documentation, the API utilizes a create_node method to initialize a node client [3][1]. The create_node method signature is as follows [3][1]: node = Bridge.create_node( connection=Bridge.NodeType.grpc, # Bridge.NodeType.grpc or Bridge.NodeType.rest address="127.0.0.1", port=2096, api_port=2097, server_ca=server_ca_pem_string, api_key="xxxxxxxx-xxxx-xxxx-xxxx-xxxxxxxxxxxx", name="node-1", # optional extra={"region": "eu-1"}, # optional default_timeout=10, # optional internal_timeout=15, # optional proxy="socks5://user:pass@127.0.0.1:1080", # optional) Parameters: connection: Specifies the connection type, either Bridge.NodeType.grpc or Bridge.NodeType.rest [3]. address: The IP address or hostname of the node [3]. port: The node service port (gRPC or protobuf-REST) [3]. api_port: The node REST JSON API port, used internally for maintenance [3]. server_ca: The PEM certificate content provided as a string [3]. api_key: A UUID string used for authentication [3]. name (optional): A name for the node logger [3]. extra (optional): A dictionary for additional metadata [3]. default_timeout (optional): Default timeout for public API methods [3]. internal_timeout (optional): Timeout for internal synchronization and log operations [3]. proxy (optional): An upstream proxy URL for node traffic [3]. max_message_size: Relevant for gRPC connections, defining HTTP/2 window or message sizing [3]. Note that PasarGuard also maintains a separate Go library called node_bridge, which uses a different API structure (e.g., node_bridge.New) [4][5]. Users should ensure they are referencing the Python-specific implementation when using pasarguard-node-bridge [1][6].
Citations:
- 1: https://github.com/pasarguard/node_bridge_py
- 2: https://github.com/PasarGuard/node_bridge_py
- 3: https://pypi.org/project/pasarguard-node-bridge/
- 4: https://github.com/pasarguard/node_bridge
- 5: https://github.com/PasarGuard/node_bridge/blob/main/README.md
- 6: https://context7.com/pasarguard/node_bridge_py/llms.txt
🏁 Script executed:
#!/bin/bash set -u printf 'Tracked dependency manifests:\n' git ls-files | rg '(^|/)(pyproject\.toml|requirements.*\.txt|Pipfile|poetry\.lock|uv\.lock|pipfile\.lock)$' || true printf '\nDependency mentions:\n' rg -n -i 'pasarguard[node_-]bridge|PasarGuardNodeBridge|node-bridge' --glob 'pyproject.toml' --glob 'requirements*.txt' --glob 'uv.lock' --glob 'poetry.lock' --glob 'Pipfile*' . || true printf '\nRelevant source imports/usages:\n' fd -a '__init__.py' . | while read -r f; do if rg -n 'create_node|_create_node_kwargs|get_bridge_memory|PasarGuardNodeBridge' "$f" >/dev/null 2>&1; then echo "--- $f" fd -a . "$f" -d 0 -x sh -c 'wc -l "{}"' rg -n -C 3 'create_node|_create_node_kwargs|get_bridge_memory|PasarGuardNodeBridge' "$f" fi doneRepository: PasarGuard/panel
Length of output: 3929
Remove unsupported
create_nodekeyword arguments.
_create_node_kwargspassesnode_id,user_sync_store,lifecycle_coordinator, andworker_idtopasarguard-node-bridge >=0.9.0create_node. That API does not accept these optional keywords in its documented signature; only standard node client fields andextraare passed through, so unknown keywords can raiseTypeErrorduring node creation.🤖 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 `@app/node/__init__.py` around lines 26 - 47, Update _create_node_kwargs to remove the unsupported node_id, user_sync_store, lifecycle_coordinator, and worker_id entries from the create_node keyword arguments. Keep the standard node client fields and extra metadata, and remove the now-unneeded get_bridge_memory call and conditional block.app/operation/node.py (1)
704-709: 🩺 Stability & Availability | 🟠 Major | 🏗️ Heavy lift
Bulk sync publishes
connectfor nodes whose local connect failed.
_connect_nodes_bulk_localfilters bynode.status, not by the connect outcome. This loop then publishes aconnectevent for every node that was not disabled or limited, including nodes that returnedNodeStatus.error.Each sibling worker consumes that event and runs
update_nodefollowed byconnect_node(seehandle_node_messageinapp/node/manager_sync.py). An unreachable node therefore receives one failed start attempt per worker instead of one. WithUVICORN_WORKERS=4and a large node set, this multiplies the retry load against nodes that are already down.Make
_connect_nodes_bulk_localreturn itsvalid_resultslist, then publish only for nodes whose result status isNodeStatus.connected.The sequential
awaitper node also serializes the publishes. Considerasyncio.gatherover the eligible node IDs.🐛 Proposed direction
async def _connect_nodes_bulk_sync(self, db: AsyncSession, nodes: list[Node]) -> None: - await self._connect_nodes_bulk_local(db, nodes) - for node in nodes: - if node is not None and node.status not in (NodeStatus.disabled, NodeStatus.limited): - await publish_node_sync("connect", node.id) + results = await self._connect_nodes_bulk_local(db, nodes) + connected_ids = [r["node_id"] for r in (results or []) if r["status"] == NodeStatus.connected] + if connected_ids: + await asyncio.gather(*(publish_node_sync("connect", node_id) for node_id in connected_ids))
_connect_nodes_bulk_localmust returnvalid_resultsfor this to work.🤖 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 `@app/operation/node.py` around lines 704 - 709, Update _connect_nodes_bulk_local to return its valid_results, then revise _connect_nodes_bulk_sync to publish connect events only for results with NodeStatus.connected, using the result’s node ID. Replace the sequential eligible-node publishing loop with asyncio.gather while preserving the existing disabled/limited filtering and avoiding publishes for failed connections.app/operation/subscription.py (1)
274-283: 🎯 Functional Correctness | 🟡 Minor | ⚡ Quick win
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash set -euo pipefail rg -n -C6 \ 'create_info_response_headers|get_format_variables|announce_url|PROFILE_TITLE' \ app testsRepository: PasarGuard/panel
Length of output: 22747
🏁 Script executed:
#!/bin/bash set -euo pipefail echo "== helper definitions ==" rg -n -C6 'def setup_format_variables|def apply_custom_format_variables|def get_effective_custom_variables|def sanitize_response_headers|def encode_title|def sub_url|BUILTIN_FORMAT_VARIABLES' app echo echo "== create_info_response_headers slice ==" sed -n '240,310p' app/operation/subscription.py echo echo "== create_response_headers slice ==" sed -n '156,193p' app/operation/subscription.py echo echo "== get_format_variables slice ==" sed -n '490,520p' app/operation/subscription.py echo echo "== tests mentioning info response headers or create_info_response_headers ==" rg -n -C5 'info|announce-url|announce_url|profile-title|profile_web_page_url|create_info_response_headers' tests app | sed -n '1,240p'Repository: PasarGuard/panel
Length of output: 29600
🏁 Script executed:
#!/bin/bash set -euo pipefail python3 - <<'PY' from collections import defaultdict from datetime import UTC, date BUILTIN_FORMAT_VARIABLES = { "SERVER_IP", "SERVER_IPV6", "USERNAME", "DATA_USAGE", "DATA_LIMIT", "DATA_LEFT", "STATUS_EMOJI", "TIME_LEFT", "STATUS", "JALALI_TIME_LEFT", "EXPIRE_DATE", "JALALI_EXPIRE_DATE", "USAGE_PERCENTAGE", "ADMIN_USERNAME", "PROFILE_TITLE", "PROTOCOL", "TRANSPORT", "url", "format", } def _custom_variable_parts(value): # Minimal mirror of app/subscription/share.py without repository imports. key = value return (key or None), "" def get_effective_custom_variables(custom_variables=None): variables = list(custom_variables or []) admin_variables = [] variables.extend(admin_variables) return variables def apply_custom_format_variables(format_variables, custom_variables=None): custom_variables = get_effective_custom_variables(custom_variables) if not custom_variables: return format_variables custom_keys = {key for key, _ in (_custom_variable_parts(variable) for variable in custom_variables) if key} base_variables = defaultdict(lambda: "<missing>", **format_variables, **custom_keys) return base_variables def setup_format_variables(user=None, custom_variables=None): base_variables = BUILTIN_FORMAT_VARIABLES.copy() format_variables = {} for key in base_variables: if key == "USERNAME": format_variables[key] = "alice" if user is None: user = type("UsersResponseWithInbounds", (), {"status": "active", "data_limit": 1000000, "used_traffic": 0, "expire": date.today()})() user.admin = None format_variables["STATUS"] = user.status format_variables["DATA_USAGE"] = user.used_traffic format_variables["DATA_LIMIT"] = str(user.data_limit / 1000000) format_variables["DATA_LEFT"] = str((user.data_limit - user.used_traffic) / 1000000) format_variables["STATUS_EMOJI"] = "(normal)" format_variables["USAGE_PERCENTAGE"] = "0.00" format_variables["TIME_LEFT"] = "∞" format_variables["JALALI_TIME_LEFT"] = "∞" format_variables["EXPIRE_DATE"] = "∞" format_variables["JALALI_EXPIRE_DATE"] = "∞" format_variables["ADMIN_USERNAME"] = str(user.admin.username if user.admin else "") return apply_custom_format_variables(format_variables, custom_variables) def get_format_variables(user=None, custom_variables=None): sub_url = "https://example.com/sub/abc" formatted_title = "Custom Title" format_variables = setup_format_variables(user, custom_variables) format_variables.update({"url": sub_url}) format_variables.update({"PROFILE_TITLE": formatted_title}) return apply_custom_format_variables(format_variables, custom_variables) custom = ["url", "PROFILE_TITLE"] info = setup_format_variables(None, custom) sub = get_format_variables(None, custom) for key in ("url", "PROFILE_TITLE"): print(f"{key}: /info={info.get(key)!r} vs subscription={sub.get(key)!r}") PYRepository: PasarGuard/panel
Length of output: 316
Keep
/infoannouncement variables consistent with subscription responses.
get_format_variablesaddsurl(subscription URL) andPROFILE_TITLE, butcreate_info_response_headersusessetup_format_variablesdirectly forannounceandannouce_url. This can make{url}and{TEMPLATE_TITLE}resolve to<missing>in/infowhile subscription responses use the completed variables. Use the same variable builder if the endpoint should support them, or update the tests/docs if this narrower contract is intentional.🤖 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 `@app/operation/subscription.py` around lines 274 - 283, Update create_info_response_headers to build format variables through get_format_variables, matching subscription response formatting so url and TEMPLATE_TITLE resolve consistently in announce and announce-url. Preserve the existing custom-variable application and formatting flow after using the shared builder.app/subscription/share.py (1)
312-312: 🩺 Stability & Availability | 🟡 Minor | ⚡ Quick win
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash set -euo pipefail rg -n -C6 'sni|format_map|validate.*sni' app testsRepository: PasarGuard/panel
Length of output: 50373
🏁 Script executed:
#!/bin/bash set -euo pipefail echo "== app/subscription/share.py around line 312 ==" sed -n '280,325p' app/subscription/share.py | cat -n echo echo "== SNI model/API/schema references ==" rg -n -C3 'class .*Host|sni:|StringArray|Field\(|validator\s*\(|@.*validat' app tests/api tests -g '*.py' | head -n 200Repository: PasarGuard/panel
Length of output: 13074
🏁 Script executed:
#!/bin/bash set -euo pipefail echo "== SNI-specific parser/formatter references ==" rg -n -C5 'sni.*format|format.*sni|HostModify|ProxyHost|TLSConfig|transport_config|sni:' app/models app/api app/core app/operation app/db app/subscription -g '*.py' | head -n 260 echo echo "== static formatter parser behavior for edge-{ and wildcards ==" python3 - <<'PY' values = ["edge-{", "edge-{USERNAME}", "*-", "no wildcards"] variables = {"USERNAME": "alice"} for value in values: after_wildcard = value.replace("*", "SALT") try: result = after_wildcard.format_map(variables) if after_wildcard else "" print(f"{value!r} -> {result!r}") except Exception as exc: print(f"{value!r} -> {type(exc).__name__}: {exc}") PYRepository: PasarGuard/panel
Length of output: 15697
🏁 Script executed:
#!/bin/bash set -euo pipefail echo "== app/models/host.py relevant snippets ==" sed -n '1,160p' app/models/host.py | cat -n sed -n '520,595p' app/models/host.py | cat -n echo echo "== validate_subscription_templates implementation ==" sed -n '35,70p' app/operation/host.py | cat -n echo echo "== string formatter behavior probe ==" python3 - <<'PY' from string import Formatter values = ["edge-{", "edge-{USERNAME}", "*-", "no wildcards"] variables = {"USERNAME": "alice"} formatter = Formatter() for value in values: after_wildcard = value.replace("*", "SALT") print(f"input={value!r}, after_wildcard={after_wildcard!r}") try: parsed = list(formatter.parse(after_wildcard)) print(f" parsed={parsed!r}") except Exception as exc: print(f" parse_error={type(exc).__name__}: {exc}") try: result = after_wildcard.format_map(variables) if after_wildcard else "" print(f" format_result={result!r}") except Exception as exc: print(f" format_error={type(exc).__name__}: {exc}") PYRepository: PasarGuard/panel
Length of output: 12831
Handle invalid SNI format strings before generating subscriptions.
app/subscription/share.py:312formats the selected SNI without a fallback. A stored value such asedge-{raisesValueErrorand aborts subscription generation. Validate SNI templates when saving, or format with a safe fallback that preserves unsupported raw values.🤖 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 `@app/subscription/share.py` at line 312, Update the SNI handling around the sni.format_map call to prevent malformed templates such as unmatched braces from aborting subscription generation. Prefer validating SNI templates when they are saved; otherwise catch formatting failures and safely retain the original raw SNI value while preserving the existing empty-string behavior.dashboard/src/features/subscriptions/components/subscription-general-settings-section.tsx (1)
139-140: 🎯 Functional Correctness | 🟠 Major | ⚡ Quick win
Give the new variable trigger an accessible name.
VariablesPopoverrenders an icon-only button withoutaria-labelor text. The newannounce_urlcontrol is therefore unnamed for screen-reader users.Add a translated
aria-labelto the sharedVariablesPopovertrigger.Proposed fix
-<Button type="button" variant="ghost" size="icon" className="h-auto w-auto p-0 hover:bg-transparent"> +<Button + type="button" + variant="ghost" + size="icon" + aria-label={t('hostsDialog.variables.title')} + className="h-auto w-auto p-0 hover:bg-transparent" +>🤖 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 `@dashboard/src/features/subscriptions/components/subscription-general-settings-section.tsx` around lines 139 - 140, Add a translated aria-label to the icon-only trigger rendered by VariablesPopover, using the existing translation mechanism and an appropriate variable-related label. Update VariablesPopover rather than CustomVariablesPopover, while preserving the current trigger behavior.dashboard/src/features/subscriptions/components/subscription-settings-schema.ts (1)
105-105: 🗄️ Data Integrity & Integration | 🟡 Minor | ⚡ Quick win
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash set -euo pipefail rg -n -C 8 'announce_url|custom_variables|replace|format' app/operation/subscription.pyRepository: PasarGuard/panel
Length of output: 23614
🏁 Script executed:
#!/bin/bash set -euo pipefail printf '%s\n' '--- subscription-settings-schema ---' cat -n dashboard/src/features/subscriptions/components/subscription-settings-schema.ts | sed -n '1,135p' printf '%s\n' '--- validate_announce_url occurrences ---' rg -n -C 6 'announce_url|announce-url|URL|urlparse|validat|raise_error' app tests . --glob '!node_modules' --glob '!dist' --glob '!build' || true printf '%s\n' '--- relevant model/schema snippets ---' rg -n -C 8 'class Subscription|SUBSettings|announce_url|announce-url|response_schema|validate_announce' app tests --glob '!node_modules' --glob '!dist' --glob '!build' || trueRepository: PasarGuard/panel
Length of output: 50373
🏁 Script executed:
#!/bin/bash set -euo pipefail printf '%s\n' '--- subscription-settings-schema.ts ---' sed -n '1,145p' dashboard/src/features/subscriptions/components/subscription-settings-schema.ts printf '%s\n' '--- app/models/settings.py relevant Subscription fields ---' rg -n -C 12 'class Subscription|announce_url|profile_title|announce' app/models/settings.py printf '%s\n' '--- app/utils/validation?url utilities ---' rg -n -C 4 'raise_error|invalid .*url|invalid.?url|urlparse|UrlParse|pydantic|model_validate|field_validator' app/utils app -g '*.py' --max-count 120 || true printf '%s\n' '--- tests mentioning announce_url or announceUrl ---' rg -n -C 8 'announce_url|announceUrl|announce-url' tests . --glob '!node_modules' --glob '!dist' --glob '!build' --max-count 160 || true printf '%s\n' '--- subscription operation lines 135-200 ---' sed -n '135,200p' app/operation/subscription.pyRepository: PasarGuard/panel
Length of output: 50373
🏁 Script executed:
#!/bin/bash set -euo pipefail printf '%s\n' '--- focused frontend schema ---' python3 - <<'PY' from pathlib import Path p=Path('dashboard/src/features/subscriptions/components/subscription-settings-schema.ts') print(f"exists={p.exists()}") if p.exists(): lines=p.read_text().splitlines() for i in range(max(0,103), min(len(lines),108)): print(f"{i+1}: {lines[i]}") PY printf '%s\n' '--- focused backend operation lines ---' python3 - <<'PY' from pathlib import Path p=Path('app/operation/subscription.py') lines=p.read_text().splitlines() for i in range(136, 201): print(f"{i+1}: {lines[i]}") PY printf '%s\n' '--- focused model definitions ---' python3 - <<'PY' from pathlib import Path for rel in ['app/models/settings.py','app/models/subscription.py']: p=Path(rel) print(f'--- {rel} ---') lines=p.read_text().splitlines() for start,end in [('class Subscription',120),('class SubRule',80),('announce_url',60),('announce',50),('profile_title',60)]: hits=[i for i,l in enumerate(lines) if start in l] for hit in hits[:3]: for j in range(max(0,hit-25), min(len(lines), hit+55)): print(f"{j+1}: {lines[j]}") print() PY printf '%s\n' '--- focused test/search files only ---' rg -n -C 8 'announce._url|announceUrl|announce-url|subscription-settings-schema|subscription_response_headers|user_subscription_raw|user_subscription_with_client_type' tests app --glob '!node_modules' --glob '!dist' --glob '!build' --max-count 200 || trueRepository: PasarGuard/panel
Length of output: 48348
Validate
announce_urlafter variable expansion.
_format_announce_url()expandssub_settings.announce_url, but onlyannounce-urlis emitted afterward. Add validation after format substitution and reject invalid expanded URLs, with a fallback when the stored value contains invalid custom variables.Add coverage for an empty value, a tokenized URL that resolves as configured, and an expanded URL that fails validation.
🤖 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 `@dashboard/src/features/subscriptions/components/subscription-settings-schema.ts` at line 105, Validate the URL produced by _format_announce_url after variable expansion, before emitting announce-url, while preserving the empty-value allowance. Reject invalid expanded URLs and fall back safely when custom variables in the stored value cannot be resolved. Add coverage for empty, successfully token-expanded, and invalid expanded URLs.pyproject.toml (1)
40-41: 🩺 Stability & Availability | 🟠 Major | ⚡ Quick win
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash set -euo pipefail rg -n -C3 \ 'pasarguard-node-bridge|index-url|extra-index-url|tool\.uv|sources' \ pyproject.toml .github 2>/dev/null || true python -m pip index versions pasarguard-node-bridgeRepository: PasarGuard/panel
Length of output: 500
🏁 Script executed:
#!/bin/bash set -euo pipefail echo "== pyproject dependency/index snippets ==" sed -n '1,80p' pyproject.toml echo echo "== repository-wide package index/source references ==" rg -n 'pypi|index-url|extra-index-url|find-links|trusted-host|tool\.uv|tool\.pdm|tool\.poetry|sources|pasarguard-node-bridge' -g '!**/.git/**' . || true echo echo "== python package tooling availability ==" command -v python || true python --version || trueRepository: PasarGuard/panel
Length of output: 50373
🌐 Web query:
pypi.org project pasarguard-node-bridge versions💡 Result:
The pasarguard-node-bridge project on PyPI maintains several historical versions. Documented versions include: - 0.8.1 [1][2] - 0.8.0 [3] - 0.7.1 [4] - 0.6.0 [5] The package is an asynchronous Python client designed to connect to a PasarGuard node using gRPC or REST protocols [1][6]. It requires Python 3.12 or higher [1]. Official documentation and source code are available through the project's GitHub repository at https://github.com/PasarGuard/node_bridge_py [1][6].
Citations:
- 1: https://pypi.org/project/pasarguard-node-bridge/
- 2: https://pypi.org/project/pasarguard-node-bridge/0.8.1/
- 3: https://pypi.org/project/pasarguard-node-bridge/0.8.0/
- 4: https://pypi.org/project/pasarguard-node-bridge/0.7.1/
- 5: https://pypi.org/project/pasarguard-node-bridge/0.6.0/
- 6: https://github.com/pasarguard/node_bridge_py
🌐 Web query:
uv.lock package "pasarguard-node-bridge" version 0.9.0 uv.lock💡 Result:
There is no evidence of a version 0.9.0 for the pasarguard-node-bridge package as of August 9, 2026. The latest available version on PyPI is 0.8.1 [1]. Technical projects utilizing this package, such as the PasarGuard panel, have been updated to use version 0.8.1 [2][3]. If you encounter a reference to version 0.9.0 in a uv.lock file, it is likely a misconfiguration or a placeholder, as this version does not exist in the official package repository [1]. You should check your pyproject.toml and uv.lock files to ensure they are pointing to a valid, released version such as 0.8.1 [1][2].
Citations:
- 1: https://pypi.org/project/pasarguard-node-bridge/
- 2: https://github.com/PasarGuard/panel/blob/main/pyproject.toml
- 3: https://newreleases.io/project/github/PasarGuard/panel/release/v4.0.0
Pin
pasarguard-node-bridgeto an available release.
pyproject.tomlrequirespasarguard-node-bridge>=0.9.0, anduv.lockuses the public PyPI registry. PyPI only lists0.8.1as the latest release, so resolver/install commands will not find a satisfying package. Lower the minimum to an available compatible version or publish0.9.0to the configured registry.🤖 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 `@pyproject.toml` around lines 40 - 41, Update the pasarguard-node-bridge dependency declaration in pyproject.toml to require the latest available compatible public PyPI release, 0.8.1, or otherwise align it with a published 0.9.0 registry release; ensure the lockfile dependency resolution remains consistent.tests/api/test_core.py (1)
60-96: 🎯 Functional Correctness | 🟡 Minor | ⚡ Quick win
Make the test fail if core creation performs reconciliation.
The unchanged
peer_ipsassertion does not distinguish pool initialization from the former full reconciliation path. The user cannot access the new inbound at this point, so both paths can leave its peer IPs unchanged.Spy on
CoreOperation._reconcile_wireguard, or instrument the reconciliation query path, and assert that WireGuard core creation does not invoke it.🤖 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 `@tests/api/test_core.py` around lines 60 - 96, Update test_wireguard_core_create_skips_user_scan_and_allocates_on_group to spy on CoreOperation._reconcile_wireguard during the create_core call and assert it is not invoked. Keep the existing peer_ips assertion, but make the test directly distinguish pool initialization from the former reconciliation path.tests/test_nats_leader_steal.py (1)
10-30: 📐 Maintainability & Code Quality | 🟡 Minor | ⚡ Quick win
Reset the
leadermodule globals after each test.Both async tests assign
leader._is_leaderandleader._tokendirectly and never restore them. Each test leavesleader._is_leader = Trueand a live token in the module singleton. Any later test in the session that readsleader.is_job_leader()sees a staleTrue, and the result depends on collection order.
tests/test_nats_leader_heartbeat.pyalready defines an autouse reset fixture. Add the same protection here.💚 Proposed fix
from app.nats import leader from role import Role +@pytest.fixture(autouse=True) +def _reset_leader_state(): + yield + leader._is_leader = False + leader._token = None + leader._kv = None + + `@pytest.mark.asyncio` async def test_try_become_leader_falls_through_to_steal_after_generic_create_error():Also applies to: 44-59
🧰 Tools
🪛 ast-grep (0.45.0)
[info] 14-14: use jsonify instead of json.dumps for JSON output
Context: json.dumps({"token": "old", "expires_at": 0})
Note: [CWE-116] Improper Encoding or Escaping of Output.(use-jsonify)
🤖 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 `@tests/test_nats_leader_steal.py` around lines 10 - 30, Add an autouse fixture in the test module that resets the leader module globals before and after each test, matching the existing pattern in test_nats_leader_heartbeat.py. Ensure _is_leader and _token are restored to their inactive values so tests involving try_become_leader do not leak singleton state.
4a7eed8 to
054cb93
Compare
d25a97d to
dfce4a1
Compare
dfce4a1 to
d9d2cfb
Compare
Summary
Validation
targeted token/revocation tests and Ruff checks passed
targeted Ruff check and formatting check
current stacked PR CI passed SQLite, PostgreSQL, TimescaleDB, MySQL, and MariaDB suites
clean stacked integration suite:
644 passed, 4 skipped; Alembic has one linear head andupgrade head/alembic checkpass.