feat: implement client template management system #286
feat: implement client template management system #286ImMohammad20000 wants to merge 17 commits intodevfrom
Conversation
- Removed legacy XRAY subscription template file and related configurations. - Introduced a new CoreTemplate model with CRUD operations for managing core templates. - Added API endpoints for creating, retrieving, modifying, and deleting core templates. - Implemented validation for template content based on type (e.g., clash, xray, singbox). - Created default templates for clash, xray, singbox, user-agent, and gRPC user-agent. - Added unit tests for core template creation, retrieval, and deletion logic.
![github-advanced-security[bot]](https://avatars.githubusercontent.com/in/57789?s=60&v=4){kind=small}
WalkthroughThis PR migrates subscription template management from environment variables and static files to a database-driven system. Templates are now stored in a Changes
Sequence DiagramsequenceDiagram
actor Client as Client/Browser
participant API as API Server
participant DB as Database
participant Cache as Template Cache
participant NATS as NATS Bus
participant Sub as Subscription Engine
Client->>API: POST /api/client_template<br/>(create template)
API->>DB: Insert ClientTemplate
API->>Cache: Invalidate cache
Cache->>NATS: Publish CLIENT_TEMPLATE event
NATS-->>Sub: Subscribe to event
Sub->>Cache: Refresh cached templates
API-->>Client: 201 + template data
Client->>Sub: Request subscription config
Sub->>Cache: Get templates (cached)
Cache-->>Sub: Template content dict
Sub->>Sub: Build config with<br/>template content
Sub-->>Client: Subscription file
Client->>API: PUT /api/client_template/{id}<br/>(modify template)
API->>DB: Update ClientTemplate
API->>Cache: Invalidate cache
Cache->>NATS: Publish CLIENT_TEMPLATE event
NATS-->>Sub: Subscribe to event
Sub->>Cache: Refresh cached templates
API-->>Client: 200 + updated data
Estimated code review effort🎯 4 (Complex) | ⏱️ ~75 minutes Poem
🚥 Pre-merge checks | ✅ 2 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ 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 Tip You can disable poems in the walkthrough.Disable the |
|
@coderabbitai review |
✅ Actions performedReview triggered.
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 10
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
app/subscription/singbox.py (1)
56-72:⚠️ Potential issue | 🔴 CriticalPipeline failure:
KeyError: 'type'when outbound entries lack a"type"field.Now that templates are user-configurable,
outbound["type"]on Lines 58/60 will raiseKeyErrorif any outbound in the loaded config is missing the"type"key. Previously, the static template guaranteed this field. Use.get("type")to guard against malformed entries.🐛 Proposed fix
def render(self, reverse=False): urltest_types = ["vmess", "vless", "trojan", "shadowsocks", "hysteria2", "tuic", "http", "ssh"] - urltest_tags = [outbound["tag"] for outbound in self.config["outbounds"] if outbound["type"] in urltest_types] + urltest_tags = [outbound["tag"] for outbound in self.config["outbounds"] if outbound.get("type") in urltest_types] selector_types = ["vmess", "vless", "trojan", "shadowsocks", "hysteria2", "tuic", "http", "ssh", "urltest"] - selector_tags = [outbound["tag"] for outbound in self.config["outbounds"] if outbound["type"] in selector_types] + selector_tags = [outbound["tag"] for outbound in self.config["outbounds"] if outbound.get("type") in selector_types]🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@app/subscription/singbox.py` around lines 56 - 72, In render(), the list comprehensions building urltest_tags and selector_tags access outbound["type"] which raises KeyError for outbounds missing the "type" key; change those to use outbound.get("type") (e.g., in the comprehensions that create urltest_tags and selector_tags) so malformed entries are skipped safely and behavior remains the same; adjust any similar direct dict indexing in render() to .get("type") to prevent the crash.
🧹 Nitpick comments (21)
app/subscription/core_templates.py (1)
7-10: Consider adding a TTL to@cached()as a safety net against stale data.
@cached()with nottlargument means the cached value never expires — invalidation relies entirely on NATS messages reachinghandle_core_template_message. If NATS delivery fails or is delayed, stale template data persists indefinitely.Adding a modest TTL (e.g., 5–10 minutes) provides a safety net:
♻️ Suggested change
-@cached() +@cached(ttl=300) async def subscription_core_templates() -> dict[str, str]:🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@app/subscription/core_templates.py` around lines 7 - 10, The cached subscription_core_templates function currently uses `@cached`() with no ttl so values never expire; add a modest TTL (e.g., ttl=300 or ttl=600) to the `@cached` decorator on subscription_core_templates to ensure the cache auto-invalidates periodically as a safety net if NATS messages to handle_core_template_message are missed or delayed, while keeping get_core_template_values and the existing manual invalidation logic intact (use the same decorator signature where subscription_core_templates is defined).app/subscription/default_templates.py (1)
18-22: Hardcoded region-specific DNS servers in default template.The
proxy-server-nameserverentries (178.22.122.100,78.157.42.100) are Shecan DNS servers specific to Iran. If this panel is intended for broader use, consider using more universally appropriate defaults (e.g.,1.1.1.1,8.8.8.8) or documenting these as region-specific.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@app/subscription/default_templates.py` around lines 18 - 22, The default template contains hardcoded region-specific DNS entries under the key "proxy-server-nameserver" (values 178.22.122.100 and 78.157.42.100); update the template in app/subscription/default_templates.py to use globally appropriate defaults (for example 1.1.1.1 and 8.8.8.8) or replace these values with clearly documented region-specific placeholders and add a comment/README note indicating they are Iran-specific so operators can override them; locate the "nameserver" and "proxy-server-nameserver" entries in the template and change the values or add the documentation accordingly.app/db/models.py (1)
717-729: Model looks well-structured.The composite unique constraint and type index align with the query patterns in the CRUD layer. Using
String(32)fortemplate_type(instead ofSQLEnum) defers validation to the application layer, which is flexible.One optional observation: unlike most other models in this file (
Admin,Node,CoreConfig, etc.),CoreTemplatelacks acreated_attimestamp. If auditing template creation/modification times is useful, consider adding one.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@app/db/models.py` around lines 717 - 729, CoreTemplate is missing a created_at timestamp—add a Mapped[datetime] mapped_column named created_at to the CoreTemplate model (class CoreTemplate) so creation time is recorded; implement it with a non-nullable DateTime column using a server default (e.g., func.now()) to auto-populate, and ensure you import DateTime and func from sqlalchemy if not already present.app/db/crud/core_template.py (6)
134-141:isinstance(s.value, tuple)branch is dead code.
CoreTemplateSortingOptionsSimplevalues are SQLAlchemyUnaryExpressionobjects (produced by.asc()/.desc()), never tuples. Thesort_list.extend(s.value)path is unreachable and can be simplified away.♻️ Proposed simplification
if sort: - sort_list = [] - for s in sort: - if isinstance(s.value, tuple): - sort_list.extend(s.value) - else: - sort_list.append(s.value) - stmt = stmt.order_by(*sort_list) + stmt = stmt.order_by(*[s.value for s in sort])🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@app/db/crud/core_template.py` around lines 134 - 141, The tuple branch is dead because CoreTemplateSortingOptionsSimple items yield SQLAlchemy UnaryExpression objects, not tuples; update the sorting logic in core_template.py to always collect s.value (e.g., replace the isinstance branch with a simple comprehension or single append per item) so sort_list becomes [s.value for s in sort] and then call stmt = stmt.order_by(*sort_list); reference symbols: sort, sort_list, s.value, and stmt.order_by.
134-141: Dead branch:isinstance(s.value, tuple)is neverTrue.
CoreTemplateSortingOptionsSimplevalues are SQLAlchemyUnaryExpressionobjects (produced by.asc()/.desc()), never tuples. Thesort_list.extend(s.value)path is unreachable and can be removed.♻️ Proposed simplification
if sort: - sort_list = [] - for s in sort: - if isinstance(s.value, tuple): - sort_list.extend(s.value) - else: - sort_list.append(s.value) - stmt = stmt.order_by(*sort_list) + stmt = stmt.order_by(*[s.value for s in sort])🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@app/db/crud/core_template.py` around lines 134 - 141, The branch checking isinstance(s.value, tuple) is dead because CoreTemplateSortingOptionsSimple produces SQLAlchemy UnaryExpression objects (from .asc()/.desc()), so remove the unreachable tuple branch and simplify the loop in the sort handling: build sort_list by appending s.value for each s in sort and then call stmt.order_by(*sort_list); update the block around sort, sort_list, s.value, and stmt.order_by accordingly to eliminate the isinstance(...) branch.
62-64: Silent DB failure inget_core_template_valuesemits no log.When the
SELECTfails withSQLAlchemyError, the function silently falls back to hardcoded defaults with no warning. Operators would have no visibility that the database is unreachable and all subscribers are being served stale defaults.♻️ Proposed fix — add a warning log
except SQLAlchemyError: + logger.warning("Failed to fetch core template values from DB; falling back to defaults", exc_info=True) return defaults(Import or reuse a module-level logger, e.g.
logger = get_logger("core-template-crud"))🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@app/db/crud/core_template.py` around lines 62 - 64, The catch in get_core_template_values currently swallows SQLAlchemyError and returns defaults silently; update the function to log a warning with the exception details before returning defaults by obtaining/using a module-level logger (e.g. define logger = get_logger("core-template-crud") at top or reuse an existing logger) and change the except block to log the error/stack (include the exception object) and then return defaults so operators can see DB failures.
108-111:if limit:skips the limit clause whenlimit=0, returning all rows instead of none.Using truthiness on an
intparameter treats0as falsy. Foroffset=0this is a no-op (harmless), butlimit=0silently drops the.limit(0)call, returning the full result set. The same pattern appears inget_core_templates_simple(lines 148–151).♻️ Proposed fix (apply identically in
get_core_templates_simplelines 148–151)- if offset: + if offset is not None: query = query.offset(offset) - if limit: + if limit is not None: query = query.limit(limit)🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@app/db/crud/core_template.py` around lines 108 - 111, The conditionals use truthiness so limit=0 is treated as False and query.limit(0) is skipped; change the checks to explicit None checks (e.g., replace "if limit:" with "if limit is not None:" and similarly "if offset:" with "if offset is not None:") in the function that builds the query and also apply the identical fix inside get_core_templates_simple so query.limit(0) and query.offset(0) are honored.
62-64: Silent DB failure inget_core_template_values— no log emitted.When the SELECT fails with
SQLAlchemyErrorthe function silently returns hardcoded defaults. Operators would have no indication that the DB is unreachable and templates have fallen back to defaults. A warning-level log before thereturn defaultswould make this observable.♻️ Proposed fix
+import logging +_logger = logging.getLogger(__name__) ... except SQLAlchemyError: + _logger.warning("Failed to fetch core template values from DB; falling back to defaults", exc_info=True) return defaults🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@app/db/crud/core_template.py` around lines 62 - 64, get_core_template_values currently swallows SQLAlchemyError and returns defaults silently; update the except block to log a warning before returning. Specifically, in get_core_template_values catch SQLAlchemyError as e (or similar), call the module logger (e.g., logger.warning or processLogger.warning) with a clear message including the exception details and context (e.g., "Failed to fetch core template values, falling back to defaults: %s" % e), then return defaults. Ensure you reference SQLAlchemyError, the defaults variable, and the get_core_template_values function when making the change.
108-111: Useis not Noneguard foroffsetandlimitto avoid incorrect behavior withlimit=0.
if offset:andif limit:treat0as falsy. Foroffset=0this is harmless (no-op), butlimit=0silently skips the limit and returns all rows instead of zero. The same pattern repeats inget_core_templates_simple(lines 148–151).♻️ Proposed fix (apply the same change in `get_core_templates_simple` lines 148–151)
- if offset: + if offset is not None: query = query.offset(offset) - if limit: + if limit is not None: query = query.limit(limit)🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@app/db/crud/core_template.py` around lines 108 - 111, Change the truthy checks for offset and limit to explicit None checks so zero values aren't treated as falsy: replace the "if offset:" and "if limit:" guards with "if offset is not None:" and "if limit is not None:" in the function containing the shown query modifications (and apply the same change in get_core_templates_simple where the same pattern occurs) so a provided limit=0 correctly applies a zero-row limit.app/models/core_template.py (2)
76-81: Inconsistent total-count field names across list response types.
CoreTemplateResponseList(line 77) calls the total countcount, whileCoreTemplatesSimpleResponse(line 94) calls ittotal. API consumers that handle both shapes will need special-case knowledge of which field to read.♻️ Proposed alignment (unify to `total`)
class CoreTemplateResponseList(BaseModel): - count: int + total: int templates: list[CoreTemplateResponse] = []Update the construction in
app/operation/core_template.pyline 95:- return CoreTemplateResponseList(templates=templates, count=count) + return CoreTemplateResponseList(templates=templates, total=count)Also applies to: 92-94
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@app/models/core_template.py` around lines 76 - 81, The list response uses inconsistent total field names: change CoreTemplateResponseList.count to CoreTemplateResponseList.total (type int) and update any construction/usages that build instances (e.g., the response created in the code that constructs CoreTemplateResponseList in the core template operations—previously setting "count") to set "total" instead; ensure CoreTemplatesSimpleResponse and CoreTemplateResponseList now share the same field name and adjust any tests/serializations that read "count" to read "total" (leave model_config as-is).
76-81: Inconsistent list-response field naming:countvstotal.
CoreTemplateResponseListexposes the size field ascount(line 77), whileCoreTemplatesSimpleResponsecalls the same concepttotal(line 94). API consumers that handle both response shapes will need to know which field name to use, which is error-prone.♻️ Proposed alignment (pick one name consistently)
class CoreTemplateResponseList(BaseModel): - count: int + total: int templates: list[CoreTemplateResponse] = [] model_config = ConfigDict(from_attributes=True)Then update the construction site in
app/operation/core_template.pyline 95:- return CoreTemplateResponseList(templates=templates, count=count) + return CoreTemplateResponseList(templates=templates, total=count)Also applies to: 92-94
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@app/models/core_template.py` around lines 76 - 81, The list response fields are inconsistent: CoreTemplateResponseList defines the size field as count while CoreTemplatesSimpleResponse uses total; pick one name (e.g., total) and make both models use it consistently by renaming the field in CoreTemplateResponseList (replace count:int with total:int) and adjusting any default/initialization (templates: list[CoreTemplateResponse] = []). Then update the code that constructs these DTOs (the place that creates a CoreTemplateResponseList instance in the core_template operation) to pass/consume the chosen field name (total) instead of count so both response shapes match.tests/api/test_core_template.py (1)
7-26: Tests don't clean up created templates.None of the test functions delete the templates they create. If the test database isn't reset between runs (or between tests in the same session), leftover templates could cause flaky results in other tests. Consider adding cleanup in a
finallyblock or using a pytest fixture with teardown.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@tests/api/test_core_template.py` around lines 7 - 26, The test_core_template_create_and_get test creates a template via create_core_template but never deletes it, risking test pollution; modify the test to remove the created template in a teardown step (e.g., wrap the create/assert logic in try/finally and call the API to delete the template by created["id"] in the finally block) or move creation to a pytest fixture that yields the created template and deletes it in the fixture teardown, ensuring cleanup uses the same client/Authorization header used in client.get.app/routers/core_template.py (3)
66-74: Unconventional route construction using"s"suffix.The list endpoints are defined as
@router.get("s")and@router.get("s/simple"), relying on the prefix/api/core_templateto form/api/core_templatesand/api/core_templates/simple. While functional, this is non-obvious and could confuse contributors. A more conventional approach would be to define the prefix as/api/core_templatesand use""/"/simple"for list routes, and"/{template_id}"for singular resource routes — or split into two routers.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@app/routers/core_template.py` around lines 66 - 74, The route decorators use an unconventional `"s"` suffix to build plural paths (e.g., `@router.get`("s") on get_core_templates) which is confusing; update the router prefix to a plural form (e.g., change the router prefix from "/api/core_template" to "/api/core_templates") and then change the list route decorators to use "" and "/simple" (e.g., `@router.get`("") for get_core_templates and `@router.get`("/simple") for get_core_templates_simple) and ensure singular routes use "/{template_id}" (e.g., get_core_template_by_id) so path construction is explicit and readable.
84-84: Parameterallshadows the Python built-in.Renaming to e.g.
fetch_allorinclude_allavoids shadowing.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@app/routers/core_template.py` at line 84, The parameter named "all" shadows the Python built-in; rename it (e.g., to include_all or fetch_all) in the function signature where "all: bool = False" appears (and update the parameter annotation) and then update every usage inside that function and all call sites to use the new name (also adjust any type hints, default handling, and tests or docs referencing the old name). Ensure the new identifier is used consistently in any related methods or templates that forwarded the parameter (search for "all=" and "all :" occurrences) to avoid runtime NameError or shadowing issues.
56-63: Returning{}from a 204 No Content endpoint.HTTP 204 semantics require no response body. Returning an empty dict is not idiomatic and may cause some clients or middleware to behave unexpectedly. Return
Noneor useResponse(status_code=204)instead.♻️ Suggested fix
`@router.delete`("/{template_id}", status_code=status.HTTP_204_NO_CONTENT) async def remove_core_template( template_id: int, db: AsyncSession = Depends(get_db), admin: AdminDetails = Depends(check_sudo_admin), ): await core_template_operator.remove_core_template(db, template_id, admin) - return {}🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@app/routers/core_template.py` around lines 56 - 63, The delete endpoint remove_core_template currently returns an empty dict which violates 204 No Content semantics; update the function (remove_core_template in app/routers/core_template.py) to not return a body — either return None or explicitly return a fastapi.Response(status_code=status.HTTP_204_NO_CONTENT) after calling core_template_operator.remove_core_template(db, template_id, admin) so no response body is emitted.app/subscription/clash.py (1)
31-31: Inconsistent None-check pattern:orvsis None.
base.pyandsingbox.pyuseif xxx is None:to default template content, but hereoris used. Withor, an empty string""would also trigger the fallback (which may be desirable), but it's inconsistent with the sibling classes. Consider aligning to one pattern across all subscription classes for predictable behavior.♻️ Suggested alignment with other classes
- self.clash_template_content = clash_template_content or DEFAULT_CLASH_SUBSCRIPTION_TEMPLATE + if clash_template_content is None: + clash_template_content = DEFAULT_CLASH_SUBSCRIPTION_TEMPLATE + self.clash_template_content = clash_template_content🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@app/subscription/clash.py` at line 31, The current assignment uses "self.clash_template_content = clash_template_content or DEFAULT_CLASH_SUBSCRIPTION_TEMPLATE", which treats empty strings as missing and differs from the "if ... is None:" pattern used in base.py and singbox.py; change the logic to mirror those classes by checking "if clash_template_content is None:" and only then set self.clash_template_content = DEFAULT_CLASH_SUBSCRIPTION_TEMPLATE (otherwise assign the provided value) so empty-string inputs remain preserved and behavior is consistent with other subscription classes.app/subscription/base.py (2)
37-37: Unnecessary explicitdelof local variables.
user_agent_dataandgrpc_user_agent_dataare local variables that will be garbage-collected when__init__returns. The explicitdeladds no value.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@app/subscription/base.py` at line 37, Remove the unnecessary explicit deletion of local variables: delete the statement "del user_agent_data, grpc_user_agent_data" from the __init__ method so that local variables user_agent_data and grpc_user_agent_data are allowed to go out of scope naturally and be garbage-collected without an explicit del.
24-30: No error handling around template rendering/parsing.If a core template stored in the DB contains invalid Jinja syntax or malformed JSON,
render_template_stringorjson.loadswill raise an unhandled exception, breaking subscription generation for all users. While validation occurs at template creation time, consider wrapping these in a try/except that falls back to the default template and logs a warning, so a corrupt template doesn't take down the subscription endpoint.🛡️ Suggested defensive fallback
+ import logging + logger = logging.getLogger(__name__) + self.proxy_remarks = [] - user_agent_data = json.loads(render_template_string(user_agent_template_content)) + try: + user_agent_data = json.loads(render_template_string(user_agent_template_content)) + except Exception: + logger.warning("Failed to render user-agent template, falling back to default") + user_agent_data = json.loads(render_template_string(DEFAULT_USER_AGENT_TEMPLATE)) if "list" in user_agent_data and isinstance(user_agent_data["list"], list): self.user_agent_list = user_agent_data["list"] else: self.user_agent_list = [] - grpc_user_agent_data = json.loads(render_template_string(grpc_user_agent_template_content)) + try: + grpc_user_agent_data = json.loads(render_template_string(grpc_user_agent_template_content)) + except Exception: + logger.warning("Failed to render gRPC user-agent template, falling back to default") + grpc_user_agent_data = json.loads(render_template_string(DEFAULT_GRPC_USER_AGENT_TEMPLATE))🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@app/subscription/base.py` around lines 24 - 30, The template rendering and JSON parsing calls (render_template_string + json.loads) for user_agent_template_content and grpc_user_agent_template_content can raise and crash subscription generation; wrap each render/json.loads block in a try/except that catches Exception, logs a warning via the class logger (e.g., self.logger.warning or whatever logger is used in this class) including which template failed and the exception, and fall back to safe defaults (set self.user_agent_list = [] and grpc_user_agent_data = {} or a default template result) so a corrupt template doesn't break the subscription endpoint; apply this defensive pattern around both the user_agent and grpc_user_agent processing in the same method.app/subscription/share.py (2)
75-76: No error handling ifsubscription_core_templates()fails.If the DB/cache call fails, the exception will propagate directly to the HTTP handler. Since this is a user-facing subscription endpoint, consider a try/except with a fallback to default templates so that subscription generation remains available even during transient DB issues.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@app/subscription/share.py` around lines 75 - 76, Wrap the call to subscription_core_templates() in a try/except to catch any exceptions, log the error (e.g., logging.exception or processLogger.error) and set a safe fallback for core_templates (for example a default templates list or an empty set) so that _build_subscription_config(config_format, core_templates) still runs and returns a usable conf; ensure you reference subscription_core_templates(), core_templates and _build_subscription_config in the change and do not let the original exception propagate to the HTTP handler.
37-65: Hard-coded dict keys are fragile — consider using constants.The template keys (
"USER_AGENT_TEMPLATE","GRPC_USER_AGENT_TEMPLATE","XRAY_SUBSCRIPTION_TEMPLATE","SINGBOX_SUBSCRIPTION_TEMPLATE","CLASH_SUBSCRIPTION_TEMPLATE") are raw string literals. A typo in any of them will produce aKeyErrorat runtime with no compile-time safety. Define these as module-level constants (or reference an enum) so they're defined once and shared withsubscription_core_templates().🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@app/subscription/share.py` around lines 37 - 65, Replace the raw string template keys in _build_subscription_config with module-level constants (e.g., USER_AGENT_TEMPLATE, GRPC_USER_AGENT_TEMPLATE, XRAY_SUBSCRIPTION_TEMPLATE, SINGBOX_SUBSCRIPTION_TEMPLATE, CLASH_SUBSCRIPTION_TEMPLATE) and use those constants when constructing common_kwargs and when passing template_*_content into StandardLinks, ClashMetaConfiguration, SingBoxConfiguration, XrayConfiguration, and OutlineConfiguration; also update the provider of core_templates (subscription_core_templates) to populate and export the same constants so both functions reference the single source of truth and avoid KeyError from mistyped literal keys.app/subscription/singbox.py (1)
19-31: Same lack of error handling asbase.pyfor template rendering.
json.loads(render_template_string(singbox_template_content))on Line 31 will raise an unhandled exception if the template content contains invalid Jinja or malformed JSON. Consider wrapping with a try/except that falls back toDEFAULT_SINGBOX_SUBSCRIPTION_TEMPLATE, consistent with the recommendation forbase.py.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@app/subscription/singbox.py` around lines 19 - 31, Wrap the template rendering and JSON parsing in SingboxSubscription.__init__ in a try/except around the call to json.loads(render_template_string(singbox_template_content)) so template/Jinja/JSON errors are caught; on exception, fall back to parsing DEFAULT_SINGBOX_SUBSCRIPTION_TEMPLATE and assign to self.config (and optionally log the error), keeping render_template_string, json.loads, DEFAULT_SINGBOX_SUBSCRIPTION_TEMPLATE and self.config as the referenced symbols to locate the change.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@app/db/migrations/versions/e8c6a4f1d2b7_add_core_templates_table.py`:
- Line 350: The gRPC User-Agent template string in the migration
e8c6a4f1d2b7_add_core_templates_table.py contains a stray ']' character inside
the JSON/seed value ("grpc-ruby/1.62.0 grpc-c/39.0.0 (osx; chttp2)]"); remove
the trailing ']' so the seeded string becomes "grpc-ruby/1.62.0 grpc-c/39.0.0
(osx; chttp2)" and update the value where it is assigned (the core
templates/default templates list in that migration) so the database seeds with
the corrected User-Agent.
- Line 350: The seeded User-Agent string in the migration
e8c6a4f1d2b7_add_core_templates_table contains a stray trailing bracket — the
value `"grpc-ruby/1.62.0 grpc-c/39.0.0 (osx; chttp2)]"` — which corrupts
persisted data; remove the trailing `]` so the seeded value becomes
`"grpc-ruby/1.62.0 grpc-c/39.0.0 (osx; chttp2)"` in the migration’s INSERT/seed
payload (leave all surrounding JSON formatting intact) and re-run the
migration/seed so the corrected string is stored.
In `@app/operation/core_template.py`:
- Around line 172-179: The deletion of a core template is being committed inside
remove_core_template while the default promotion happens afterwards in
set_default_template, which can leave the type without a default on failure;
change the flow so both delete and promotion occur in a single DB transaction:
either modify remove_core_template to only call db.delete() without committing
and let the caller (the code around get_first_template_by_type /
set_default_template) perform the transaction/commit, or implement a new atomic
helper (e.g., remove_core_template_and_promote_default) that runs delete +
set_default_template logic inside one transactional unit and commits once both
succeed; update callers to use the new transactional behavior and ensure no
intermediate commit happens in remove_core_template.
- Around line 172-179: The deletion commits separately causing a window where a
failure in promotion leaves the type without a default; change the flow so
deletion and promotion occur in one transaction: update remove_core_template to
only call db.delete(db_template) (no db.commit()) or add a non-committing
variant, then in the caller (the function handling db_template removal) call
get_first_template_by_type (if db_template.is_default) and set_default_template
as needed, and finally perform a single db.commit() once both deletion and
default promotion succeed; references: remove_core_template,
set_default_template, get_first_template_by_type, db_template, replacement.
- Around line 46-66: The JSON validation branch in _validate_template_content
calls render_template_string(content) with no context so undefined Jinja
variables render to empty strings and json.loads can fail; update the JSON
branch in _validate_template_content to call render_template_string with a
minimal stub context matching the JSON template kinds (e.g., provide keys like
outbounds: [], inbounds: [], proxies: [], proxy-groups: [], rules: [],
proxy_remarks: [] as appropriate) before json.loads, or alternatively skip
rendering and validate raw JSON structure first; ensure changes reference
CoreTemplateType and keep the existing parsed-type check (e.g., for
CoreTemplateType.user_agent and CoreTemplateType.grpc_user_agent) so templates
still must render to a JSON object.
- Around line 46-66: The JSON template validation in _validate_template_content
is inconsistent: only CoreTemplateType.clash_subscription gets a stub Jinja2
context before rendering, causing other types (e.g., Xray, SingBox, user_agent,
grpc_user_agent) to fail on templates that reference variables; update the
render call(s) inside _validate_template_content so all template types use an
appropriate mock context (e.g., the same {"conf": {"proxies": [],
"proxy-groups": [], "rules": []}, "proxy_remarks": []} or a minimal common stub)
when calling render_template_string for non-clash types, then parse the rendered
output (yaml.safe_load or json.loads as appropriate) and keep the existing
exception handling and raise_error behavior.
In `@app/subscription/share.py`:
- Around line 58-59: OutlineConfiguration currently subclasses BaseSubscription
but its __init__ does not call super().__init__, leaving the parent
uninitialized; either remove the inheritance or update
OutlineConfiguration.__init__ to call
super().__init__(user_agent_template_content, grpc_user_agent_template_content)
and properly store/handle those template args, and update the instantiation site
(the branch that returns OutlineConfiguration()) to pass the needed template
content variables so BaseSubscription is initialized correctly; refer to the
OutlineConfiguration class and BaseSubscription.__init__ for where to add the
super call and to the place that constructs OutlineConfiguration to supply the
two template parameters.
In `@app/templates/__init__.py`:
- Around line 24-25: The render_template_string function currently calls
env.from_string(template_content) which renders untrusted DB-sourced templates
with an unsandboxed Jinja2 Environment; replace this with a
SandboxedEnvironment-based render: create or use
jinja2.sandbox.SandboxedEnvironment to parse and render template_content
(preserving the same context fallback logic), so that render_template_string
uses SandboxedEnvironment.from_string(...).render(context or {}) instead of
env.from_string(...).render(...), ensuring the template execution is sandboxed
for defense-in-depth while keeping the function signature and context handling
intact.
In `@tests/api/test_core_template.py`:
- Around line 29-54: The test test_core_template_can_switch_default is brittle
because DB migrations may seed a default template for template_type
"singbox_subscription"; instead first query any existing default for that type
(e.g., GET
/api/core_template?template_type=singbox_subscription&is_default=true) or create
a template and explicitly set it as default via create_core_template to
guarantee a known previous default, then create the second template with
is_default=True using create_core_template and finally fetch both templates
(using client.get on the IDs created) to assert the original default is now
is_default False and the new one is True; update references to
create_core_template, test_core_template_can_switch_default, and the client.get
checks accordingly.
- Around line 57-88: The test test_core_template_last_and_system_delete_guards
assumes no seeded templates for template_type "grpc_user_agent"; update the test
to avoid seeded types by using a unique/non-seeded template_type (e.g., change
template_type passed to create_core_template in that test) or first query
existing templates for that type and adjust the expected responses accordingly
so the delete-guard assertions reflect whether a system/last template already
exists; modify references in the test function
(test_core_template_last_and_system_delete_guards and the two
create_core_template calls) to use the new non-seeded type or include a
pre-check that accounts for seeded data before asserting 403/204.
---
Outside diff comments:
In `@app/subscription/singbox.py`:
- Around line 56-72: In render(), the list comprehensions building urltest_tags
and selector_tags access outbound["type"] which raises KeyError for outbounds
missing the "type" key; change those to use outbound.get("type") (e.g., in the
comprehensions that create urltest_tags and selector_tags) so malformed entries
are skipped safely and behavior remains the same; adjust any similar direct dict
indexing in render() to .get("type") to prevent the crash.
---
Nitpick comments:
In `@app/db/crud/core_template.py`:
- Around line 134-141: The tuple branch is dead because
CoreTemplateSortingOptionsSimple items yield SQLAlchemy UnaryExpression objects,
not tuples; update the sorting logic in core_template.py to always collect
s.value (e.g., replace the isinstance branch with a simple comprehension or
single append per item) so sort_list becomes [s.value for s in sort] and then
call stmt = stmt.order_by(*sort_list); reference symbols: sort, sort_list,
s.value, and stmt.order_by.
- Around line 134-141: The branch checking isinstance(s.value, tuple) is dead
because CoreTemplateSortingOptionsSimple produces SQLAlchemy UnaryExpression
objects (from .asc()/.desc()), so remove the unreachable tuple branch and
simplify the loop in the sort handling: build sort_list by appending s.value for
each s in sort and then call stmt.order_by(*sort_list); update the block around
sort, sort_list, s.value, and stmt.order_by accordingly to eliminate the
isinstance(...) branch.
- Around line 62-64: The catch in get_core_template_values currently swallows
SQLAlchemyError and returns defaults silently; update the function to log a
warning with the exception details before returning defaults by obtaining/using
a module-level logger (e.g. define logger = get_logger("core-template-crud") at
top or reuse an existing logger) and change the except block to log the
error/stack (include the exception object) and then return defaults so operators
can see DB failures.
- Around line 108-111: The conditionals use truthiness so limit=0 is treated as
False and query.limit(0) is skipped; change the checks to explicit None checks
(e.g., replace "if limit:" with "if limit is not None:" and similarly "if
offset:" with "if offset is not None:") in the function that builds the query
and also apply the identical fix inside get_core_templates_simple so
query.limit(0) and query.offset(0) are honored.
- Around line 62-64: get_core_template_values currently swallows SQLAlchemyError
and returns defaults silently; update the except block to log a warning before
returning. Specifically, in get_core_template_values catch SQLAlchemyError as e
(or similar), call the module logger (e.g., logger.warning or
processLogger.warning) with a clear message including the exception details and
context (e.g., "Failed to fetch core template values, falling back to defaults:
%s" % e), then return defaults. Ensure you reference SQLAlchemyError, the
defaults variable, and the get_core_template_values function when making the
change.
- Around line 108-111: Change the truthy checks for offset and limit to explicit
None checks so zero values aren't treated as falsy: replace the "if offset:" and
"if limit:" guards with "if offset is not None:" and "if limit is not None:" in
the function containing the shown query modifications (and apply the same change
in get_core_templates_simple where the same pattern occurs) so a provided
limit=0 correctly applies a zero-row limit.
In `@app/db/models.py`:
- Around line 717-729: CoreTemplate is missing a created_at timestamp—add a
Mapped[datetime] mapped_column named created_at to the CoreTemplate model (class
CoreTemplate) so creation time is recorded; implement it with a non-nullable
DateTime column using a server default (e.g., func.now()) to auto-populate, and
ensure you import DateTime and func from sqlalchemy if not already present.
In `@app/models/core_template.py`:
- Around line 76-81: The list response uses inconsistent total field names:
change CoreTemplateResponseList.count to CoreTemplateResponseList.total (type
int) and update any construction/usages that build instances (e.g., the response
created in the code that constructs CoreTemplateResponseList in the core
template operations—previously setting "count") to set "total" instead; ensure
CoreTemplatesSimpleResponse and CoreTemplateResponseList now share the same
field name and adjust any tests/serializations that read "count" to read "total"
(leave model_config as-is).
- Around line 76-81: The list response fields are inconsistent:
CoreTemplateResponseList defines the size field as count while
CoreTemplatesSimpleResponse uses total; pick one name (e.g., total) and make
both models use it consistently by renaming the field in
CoreTemplateResponseList (replace count:int with total:int) and adjusting any
default/initialization (templates: list[CoreTemplateResponse] = []). Then update
the code that constructs these DTOs (the place that creates a
CoreTemplateResponseList instance in the core_template operation) to
pass/consume the chosen field name (total) instead of count so both response
shapes match.
In `@app/routers/core_template.py`:
- Around line 66-74: The route decorators use an unconventional `"s"` suffix to
build plural paths (e.g., `@router.get`("s") on get_core_templates) which is
confusing; update the router prefix to a plural form (e.g., change the router
prefix from "/api/core_template" to "/api/core_templates") and then change the
list route decorators to use "" and "/simple" (e.g., `@router.get`("") for
get_core_templates and `@router.get`("/simple") for get_core_templates_simple) and
ensure singular routes use "/{template_id}" (e.g., get_core_template_by_id) so
path construction is explicit and readable.
- Line 84: The parameter named "all" shadows the Python built-in; rename it
(e.g., to include_all or fetch_all) in the function signature where "all: bool =
False" appears (and update the parameter annotation) and then update every usage
inside that function and all call sites to use the new name (also adjust any
type hints, default handling, and tests or docs referencing the old name).
Ensure the new identifier is used consistently in any related methods or
templates that forwarded the parameter (search for "all=" and "all :"
occurrences) to avoid runtime NameError or shadowing issues.
- Around line 56-63: The delete endpoint remove_core_template currently returns
an empty dict which violates 204 No Content semantics; update the function
(remove_core_template in app/routers/core_template.py) to not return a body —
either return None or explicitly return a
fastapi.Response(status_code=status.HTTP_204_NO_CONTENT) after calling
core_template_operator.remove_core_template(db, template_id, admin) so no
response body is emitted.
In `@app/subscription/base.py`:
- Line 37: Remove the unnecessary explicit deletion of local variables: delete
the statement "del user_agent_data, grpc_user_agent_data" from the __init__
method so that local variables user_agent_data and grpc_user_agent_data are
allowed to go out of scope naturally and be garbage-collected without an
explicit del.
- Around line 24-30: The template rendering and JSON parsing calls
(render_template_string + json.loads) for user_agent_template_content and
grpc_user_agent_template_content can raise and crash subscription generation;
wrap each render/json.loads block in a try/except that catches Exception, logs a
warning via the class logger (e.g., self.logger.warning or whatever logger is
used in this class) including which template failed and the exception, and fall
back to safe defaults (set self.user_agent_list = [] and grpc_user_agent_data =
{} or a default template result) so a corrupt template doesn't break the
subscription endpoint; apply this defensive pattern around both the user_agent
and grpc_user_agent processing in the same method.
In `@app/subscription/clash.py`:
- Line 31: The current assignment uses "self.clash_template_content =
clash_template_content or DEFAULT_CLASH_SUBSCRIPTION_TEMPLATE", which treats
empty strings as missing and differs from the "if ... is None:" pattern used in
base.py and singbox.py; change the logic to mirror those classes by checking "if
clash_template_content is None:" and only then set self.clash_template_content =
DEFAULT_CLASH_SUBSCRIPTION_TEMPLATE (otherwise assign the provided value) so
empty-string inputs remain preserved and behavior is consistent with other
subscription classes.
In `@app/subscription/core_templates.py`:
- Around line 7-10: The cached subscription_core_templates function currently
uses `@cached`() with no ttl so values never expire; add a modest TTL (e.g.,
ttl=300 or ttl=600) to the `@cached` decorator on subscription_core_templates to
ensure the cache auto-invalidates periodically as a safety net if NATS messages
to handle_core_template_message are missed or delayed, while keeping
get_core_template_values and the existing manual invalidation logic intact (use
the same decorator signature where subscription_core_templates is defined).
In `@app/subscription/default_templates.py`:
- Around line 18-22: The default template contains hardcoded region-specific DNS
entries under the key "proxy-server-nameserver" (values 178.22.122.100 and
78.157.42.100); update the template in app/subscription/default_templates.py to
use globally appropriate defaults (for example 1.1.1.1 and 8.8.8.8) or replace
these values with clearly documented region-specific placeholders and add a
comment/README note indicating they are Iran-specific so operators can override
them; locate the "nameserver" and "proxy-server-nameserver" entries in the
template and change the values or add the documentation accordingly.
In `@app/subscription/share.py`:
- Around line 75-76: Wrap the call to subscription_core_templates() in a
try/except to catch any exceptions, log the error (e.g., logging.exception or
processLogger.error) and set a safe fallback for core_templates (for example a
default templates list or an empty set) so that
_build_subscription_config(config_format, core_templates) still runs and returns
a usable conf; ensure you reference subscription_core_templates(),
core_templates and _build_subscription_config in the change and do not let the
original exception propagate to the HTTP handler.
- Around line 37-65: Replace the raw string template keys in
_build_subscription_config with module-level constants (e.g.,
USER_AGENT_TEMPLATE, GRPC_USER_AGENT_TEMPLATE, XRAY_SUBSCRIPTION_TEMPLATE,
SINGBOX_SUBSCRIPTION_TEMPLATE, CLASH_SUBSCRIPTION_TEMPLATE) and use those
constants when constructing common_kwargs and when passing template_*_content
into StandardLinks, ClashMetaConfiguration, SingBoxConfiguration,
XrayConfiguration, and OutlineConfiguration; also update the provider of
core_templates (subscription_core_templates) to populate and export the same
constants so both functions reference the single source of truth and avoid
KeyError from mistyped literal keys.
In `@app/subscription/singbox.py`:
- Around line 19-31: Wrap the template rendering and JSON parsing in
SingboxSubscription.__init__ in a try/except around the call to
json.loads(render_template_string(singbox_template_content)) so
template/Jinja/JSON errors are caught; on exception, fall back to parsing
DEFAULT_SINGBOX_SUBSCRIPTION_TEMPLATE and assign to self.config (and optionally
log the error), keeping render_template_string, json.loads,
DEFAULT_SINGBOX_SUBSCRIPTION_TEMPLATE and self.config as the referenced symbols
to locate the change.
In `@tests/api/test_core_template.py`:
- Around line 7-26: The test_core_template_create_and_get test creates a
template via create_core_template but never deletes it, risking test pollution;
modify the test to remove the created template in a teardown step (e.g., wrap
the create/assert logic in try/finally and call the API to delete the template
by created["id"] in the finally block) or move creation to a pytest fixture that
yields the created template and deletes it in the fixture teardown, ensuring
cleanup uses the same client/Authorization header used in client.get.
app/operation/core_template.py
Outdated
| async def _validate_template_content(self, template_type: CoreTemplateType, content: str) -> None: | ||
| try: | ||
| if template_type == CoreTemplateType.clash_subscription: | ||
| rendered = render_template_string( | ||
| content, | ||
| { | ||
| "conf": {"proxies": [], "proxy-groups": [], "rules": []}, | ||
| "proxy_remarks": [], | ||
| }, | ||
| ) | ||
| yaml.safe_load(rendered) | ||
| return | ||
|
|
||
| rendered = render_template_string(content) | ||
| parsed = json.loads(rendered) | ||
| if template_type in (CoreTemplateType.user_agent, CoreTemplateType.grpc_user_agent) and not isinstance( | ||
| parsed, dict | ||
| ): | ||
| raise ValueError("User-Agent template content must render to a JSON object") | ||
| except Exception as exc: | ||
| await self.raise_error(message=f"Invalid template content: {str(exc)}", code=400) |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
# Find render_template_string definition in app/templates/
find app/templates -type f -name "*.py" | head -20Repository: PasarGuard/panel
Length of output: 110
🏁 Script executed:
# Look for the render_template_string function definition
rg -n "def render_template_string" --type pyRepository: PasarGuard/panel
Length of output: 181
🏁 Script executed:
# Search for Jinja2 undefined mode configuration
rg -n "undefined|StrictUndefined|ChainableUndefined|DebugUndefined" --type py app/templates/Repository: PasarGuard/panel
Length of output: 42
🏁 Script executed:
cat -n app/templates/__init__.pyRepository: PasarGuard/panel
Length of output: 1067
🏁 Script executed:
sed -n '46,66p' app/operation/core_template.pyRepository: PasarGuard/panel
Length of output: 1056
JSON template validation skips context — valid templates using undefined variables will be silently rejected.
The Clash branch (lines 49–55) passes a mock context to render_template_string, ensuring template variables resolve correctly. The JSON branch (line 59) calls render_template_string(content) with no context, resulting in an empty dict. Since the Jinja2 Environment uses default undefined mode (app/templates/init.py:15), undefined variables render as empty strings. For custom Xray or SingBox templates using any undefined variable (e.g., {{ outbounds | tojson }}), this produces malformed JSON that fails json.loads, incorrectly rejecting valid templates.
Provide minimal stub contexts for each JSON type (e.g., an empty outbounds list for Xray/SingBox) — mirroring the Clash approach — or skip validation entirely for JSON types and validate raw JSON structure before rendering.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@app/operation/core_template.py` around lines 46 - 66, The JSON validation
branch in _validate_template_content calls render_template_string(content) with
no context so undefined Jinja variables render to empty strings and json.loads
can fail; update the JSON branch in _validate_template_content to call
render_template_string with a minimal stub context matching the JSON template
kinds (e.g., provide keys like outbounds: [], inbounds: [], proxies: [],
proxy-groups: [], rules: [], proxy_remarks: [] as appropriate) before
json.loads, or alternatively skip rendering and validate raw JSON structure
first; ensure changes reference CoreTemplateType and keep the existing
parsed-type check (e.g., for CoreTemplateType.user_agent and
CoreTemplateType.grpc_user_agent) so templates still must render to a JSON
object.
🧩 Analysis chain
🌐 Web query:
Jinja2 default behavior undefined variables UndefinedError render empty string
💡 Result:
In Jinja2’s default configuration, missing names/attributes resolve to an instance of jinja2.Undefined:
{{ missing }}renders as an empty string ('') and is falsy.- But many other operations on it (math, string ops, etc.) will raise
jinja2.exceptions.UndefinedError.
This is the documented behavior ofUndefined. [1]
If you want an error instead of rendering '', switch to StrictUndefined:
from jinja2 import Environment, StrictUndefined
env = Environment(undefined=StrictUndefined)(or in Flask: app.jinja_env.undefined = StrictUndefined). [1][2]
If you want to keep default undefined behavior but provide a fallback value in the template, use the default filter:
{{ missing | default('') }}(default returns the fallback when the value is undefined; it can also be configured to treat falsy values as needing the default via its second parameter). [3]
Sources:
[1] Jinja “API” docs – Undefined / StrictUndefined behavior and UndefinedError
[2] Stack Overflow – setting Flask to StrictUndefined
[3] Jinja “Template Designer Documentation” – default filter behavior
🏁 Script executed:
fd -type f -name "*.py" | xargs rg "def render_template_string" -A 20Repository: PasarGuard/panel
Length of output: 459
🏁 Script executed:
fd -type f -name "*.py" | xargs rg "Environment\(" -B 2 -A 10 | head -100Repository: PasarGuard/panel
Length of output: 1109
🏁 Script executed:
rg "from jinja2|import.*Jinja" --type pyRepository: PasarGuard/panel
Length of output: 42
🏁 Script executed:
rg "CoreTemplateType\.(xray|singbox|user_agent|grpc_user_agent)" -B 2 -A 2 | head -50Repository: PasarGuard/panel
Length of output: 1110
🏁 Script executed:
fd -type f -name "*.py" | xargs rg "def.*test.*template|def.*test.*validate" | head -20Repository: PasarGuard/panel
Length of output: 1992
🏁 Script executed:
rg "clash_subscription|user_agent.*template" -B 3 -A 3 | head -80Repository: PasarGuard/panel
Length of output: 5113
🏁 Script executed:
cat tests/api/test_core_template.pyRepository: PasarGuard/panel
Length of output: 2851
🏁 Script executed:
rg "DEFAULT_.*TEMPLATE|DEFAULT_XRAY|DEFAULT_SINGBOX|DEFAULT_USER_AGENT" -A 5Repository: PasarGuard/panel
Length of output: 12861
JSON template validation inconsistent with Clash—other types lack stub context for Jinja2 variables.
_validate_template_content renders Clash templates with mock context (lines 49–55) to properly resolve Jinja2 variables. For Xray, SingBox, and user-agent types (lines 59–64), render_template_string(content) is called with no context. If a custom template contains Jinja2 expressions (e.g., {{ proxies | tojson }}), undefined variables render as empty strings or raise UndefinedError, producing invalid JSON and incorrectly rejecting the template. The render infrastructure is present for all types, so templates should either all receive mock/stub context to be consistent with Clash, or validation should be relaxed to check raw JSON structure before rendering.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@app/operation/core_template.py` around lines 46 - 66, The JSON template
validation in _validate_template_content is inconsistent: only
CoreTemplateType.clash_subscription gets a stub Jinja2 context before rendering,
causing other types (e.g., Xray, SingBox, user_agent, grpc_user_agent) to fail
on templates that reference variables; update the render call(s) inside
_validate_template_content so all template types use an appropriate mock context
(e.g., the same {"conf": {"proxies": [], "proxy-groups": [], "rules": []},
"proxy_remarks": []} or a minimal common stub) when calling
render_template_string for non-clash types, then parse the rendered output
(yaml.safe_load or json.loads as appropriate) and keep the existing exception
handling and raise_error behavior.
| replacement = None | ||
| if db_template.is_default: | ||
| replacement = await get_first_template_by_type(db, template_type, exclude_id=db_template.id) | ||
|
|
||
| await remove_core_template(db, db_template) | ||
|
|
||
| if replacement is not None: | ||
| await set_default_template(db, replacement) |
There was a problem hiding this comment.
Delete and default-promotion are in separate transactions — orphaned state on failure.
remove_core_template (line 176) commits the deletion immediately. If set_default_template (line 179) subsequently fails (e.g. transient DB error), the type has no default template until a manual fix. Combine both operations in a single transaction: have remove_core_template only issue a db.delete() without committing, and perform the set_default_template logic in the same unit of work before the shared commit.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@app/operation/core_template.py` around lines 172 - 179, The deletion of a
core template is being committed inside remove_core_template while the default
promotion happens afterwards in set_default_template, which can leave the type
without a default on failure; change the flow so both delete and promotion occur
in a single DB transaction: either modify remove_core_template to only call
db.delete() without committing and let the caller (the code around
get_first_template_by_type / set_default_template) perform the
transaction/commit, or implement a new atomic helper (e.g.,
remove_core_template_and_promote_default) that runs delete +
set_default_template logic inside one transactional unit and commits once both
succeed; update callers to use the new transactional behavior and ensure no
intermediate commit happens in remove_core_template.
Delete and default-promotion are in separate transactions — leaves type without a default on failure.
remove_core_template (line 176) calls db.commit() inside the CRUD function, so the deletion is permanently committed. If set_default_template (line 179) subsequently fails (e.g. transient DB error during its own db.commit()), the type is left with no default template and no automatic recovery path.
Refactor both steps into a single unit of work: have remove_core_template issue db.delete() without committing, then perform the default-promotion before the shared final commit.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@app/operation/core_template.py` around lines 172 - 179, The deletion commits
separately causing a window where a failure in promotion leaves the type without
a default; change the flow so deletion and promotion occur in one transaction:
update remove_core_template to only call db.delete(db_template) (no db.commit())
or add a non-committing variant, then in the caller (the function handling
db_template removal) call get_first_template_by_type (if db_template.is_default)
and set_default_template as needed, and finally perform a single db.commit()
once both deletion and default promotion succeed; references:
remove_core_template, set_default_template, get_first_template_by_type,
db_template, replacement.
| if config_format == "outline": | ||
| return OutlineConfiguration() |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
# Find and examine OutlineConfiguration class definition and its parent
rg -n 'class OutlineConfiguration' --type=py -A 15Repository: PasarGuard/panel
Length of output: 1118
🏁 Script executed:
# Check the share.py file for all config instantiations to compare
rg -n 'Configuration\(\)' app/subscription/share.pyRepository: PasarGuard/panel
Length of output: 100
🏁 Script executed:
# Examine the BaseSubscription class to understand template handling
rg -n 'class BaseSubscription' --type=py -A 20Repository: PasarGuard/panel
Length of output: 1592
🏁 Script executed:
# Look at all config instantiation patterns in the file
sed -n '40,80p' app/subscription/share.pyRepository: PasarGuard/panel
Length of output: 1608
🏁 Script executed:
# View the complete OutlineConfiguration class to check for super().__init__() call
cat app/subscription/outline.pyRepository: PasarGuard/panel
Length of output: 1487
🏁 Script executed:
# Check if OutlineConfiguration uses any BaseSubscription attributes like user_agent_list
rg -n 'self\.user_agent' app/subscription/outline.py
rg -n 'self\.proxy_remarks' app/subscription/outline.py
rg -n 'self\.grpc_user_agent' app/subscription/outline.pyRepository: PasarGuard/panel
Length of output: 42
🏁 Script executed:
# Check the full function signature and context of _build_subscription_config
sed -n '45,70p' app/subscription/share.py | cat -nRepository: PasarGuard/panel
Length of output: 1098
OutlineConfiguration doesn't call super().__init__() — this is a design issue.
OutlineConfiguration extends BaseSubscription but its __init__() never calls the parent constructor. While it's instantiated bare without template parameters in share.py line 59 (unlike all other config types), the actual problem is deeper: OutlineConfiguration doesn't initialize BaseSubscription at all. This creates misleading inheritance — either OutlineConfiguration should not extend BaseSubscription, or it should call super().__init__(user_agent_template_content, grpc_user_agent_template_content) and handle templates properly. Currently, it works only because OutlineConfiguration doesn't use any parent class attributes, but this is fragile design.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@app/subscription/share.py` around lines 58 - 59, OutlineConfiguration
currently subclasses BaseSubscription but its __init__ does not call
super().__init__, leaving the parent uninitialized; either remove the
inheritance or update OutlineConfiguration.__init__ to call
super().__init__(user_agent_template_content, grpc_user_agent_template_content)
and properly store/handle those template args, and update the instantiation site
(the branch that returns OutlineConfiguration()) to pass the needed template
content variables so BaseSubscription is initialized correctly; refer to the
OutlineConfiguration class and BaseSubscription.__init__ for where to add the
super call and to the place that constructs OutlineConfiguration to supply the
two template parameters.
| def render_template_string(template_content: str, context: Union[dict, None] = None) -> str: | ||
| return env.from_string(template_content).render(context or {}) |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
# First, view the actual app/templates/__init__.py file to see the env setup and function definition
cat -n app/templates/__init__.pyRepository: PasarGuard/panel
Length of output: 1067
🏁 Script executed:
# Search for all calls to render_template_string to understand usage patterns
rg -n 'render_template_string' --type=py -B 2 -A 2Repository: PasarGuard/panel
Length of output: 4534
🏁 Script executed:
# Check the CoreTemplate model definition to understand data flow
rg -n 'class CoreTemplate' --type=py -A 15Repository: PasarGuard/panel
Length of output: 6503
🏁 Script executed:
# Look at the admin API endpoint that modifies template content
rg -n 'core_template' app/routers/ --type=py -B 2 -A 5Repository: PasarGuard/panel
Length of output: 6574
🏁 Script executed:
# Check the check_sudo_admin implementation
rg -n 'def check_sudo_admin' --type=py -A 10Repository: PasarGuard/panel
Length of output: 996
🏁 Script executed:
# Search for DEFAULT_*_TEMPLATE definitions and usage
rg -n 'DEFAULT_.*TEMPLATE' --type=py -B 2 -A 2Repository: PasarGuard/panel
Length of output: 10574
🏁 Script executed:
# Check if CoreTemplate content can be modified outside the router API
rg -n 'CoreTemplate' --type=py | grep -i 'update\|modify\|content.*='Repository: PasarGuard/panel
Length of output: 793
Server-Side Template Injection (SSTI) risk with unsandboxed Jinja2 environment.
The SSTI vulnerability is valid: env.from_string(template_content) renders database-sourced template content through an unsandboxed Jinja2 Environment, allowing arbitrary Python code execution via template payloads (e.g., {{ ''.__class__.__mro__... }}).
However, note that CoreTemplate content can only be modified by sudo admins (all create/modify endpoints require check_sudo_admin). The risk exists but is narrower in scope than general admin escalation—it requires either a compromised sudo admin account or a malicious insider with sudo privileges.
Consider using a SandboxedEnvironment for defense-in-depth:
🔒 Proposed fix using SandboxedEnvironment
+from jinja2.sandbox import SandboxedEnvironment
+
+sandboxed_env = SandboxedEnvironment()
+sandboxed_env.filters.update(CUSTOM_FILTERS)
+sandboxed_env.globals["now"] = lambda: dt.now(tz.utc)
+
+
def render_template_string(template_content: str, context: Union[dict, None] = None) -> str:
- return env.from_string(template_content).render(context or {})
+ return sandboxed_env.from_string(template_content).render(context or {})📝 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.
| def render_template_string(template_content: str, context: Union[dict, None] = None) -> str: | |
| return env.from_string(template_content).render(context or {}) | |
| from jinja2.sandbox import SandboxedEnvironment | |
| sandboxed_env = SandboxedEnvironment() | |
| sandboxed_env.filters.update(CUSTOM_FILTERS) | |
| sandboxed_env.globals["now"] = lambda: dt.now(tz.utc) | |
| def render_template_string(template_content: str, context: Union[dict, None] = None) -> str: | |
| return sandboxed_env.from_string(template_content).render(context or {}) |
🧰 Tools
🪛 GitHub Check: CodeQL
[failure] 25-25: Server Side Template Injection
This template construction depends on a user-provided value.
This template construction depends on a user-provided value.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@app/templates/__init__.py` around lines 24 - 25, The render_template_string
function currently calls env.from_string(template_content) which renders
untrusted DB-sourced templates with an unsandboxed Jinja2 Environment; replace
this with a SandboxedEnvironment-based render: create or use
jinja2.sandbox.SandboxedEnvironment to parse and render template_content
(preserving the same context fallback logic), so that render_template_string
uses SandboxedEnvironment.from_string(...).render(context or {}) instead of
env.from_string(...).render(...), ensuring the template execution is sandboxed
for defense-in-depth while keeping the function signature and context handling
intact.
ImMohammad20000
force-pushed
the
core-templates
branch
from
c80013a to
9cc26bd
Compare
ImMohammad20000
changed the title
- Implemented mobile YAML Ace Editor for editing YAML content. - Created ClientTemplateModal for adding and editing client templates with validation. - Defined client template form schema and default values using Zod. - Developed ClientTemplateActionsMenu for editing, duplicating, and deleting client templates. - Created ClientTemplate component to display individual templates with action menu. - Added useClientTemplatesListColumns hook for managing template list columns. - Implemented ClientTemplates page for listing, searching, and managing client templates. - Introduced UserTemplates page for managing user templates with similar functionalities.
|
@coderabbitai review |
✅ Actions performedReview triggered.
|
There was a problem hiding this comment.
Actionable comments posted: 13
🧹 Nitpick comments (3)
dashboard/src/components/templates/client-template-actions-menu.tsx (1)
151-161: Type mismatch inhandleDeleteClickevent parameter.The
onSelectcallback ofDropdownMenuItemreceives aReact.SyntheticEvent, buthandleDeleteClickexpects a DOMEvent. While this works at runtime due to duck typing, the type annotation is imprecise.Proposed fix
- const handleDeleteClick = (event: Event) => { + const handleDeleteClick = (event: React.SyntheticEvent | Event) => { event.preventDefault() event.stopPropagation() setDeleteDialogOpen(true) }Or simply inline the handler since it's only used once:
<DropdownMenuItem dir={dir} className="!text-red-500" - onSelect={e => { - e.stopPropagation() - handleDeleteClick(e) - }} + onSelect={e => { + e.preventDefault() + e.stopPropagation() + setDeleteDialogOpen(true) + }} >🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@dashboard/src/components/templates/client-template-actions-menu.tsx` around lines 151 - 161, The onSelect handler for DropdownMenuItem is passing a React.SyntheticEvent but handleDeleteClick is typed to accept a DOM Event; update the types to match by changing handleDeleteClick's parameter type to React.SyntheticEvent (or Event | React.SyntheticEvent) so signatures align, then call e.stopPropagation() and pass e to handleDeleteClick; alternatively inline the delete logic in the onSelect callback and remove the mismatched handleDeleteClick parameter to avoid the type mismatch (referencing DropdownMenuItem, onSelect, and handleDeleteClick).app/operation/client_template.py (1)
122-122: Parameterallshadows Python built-in.Using
allas a parameter name shadows the built-in function. While it works correctly here, it can cause confusion and subtle bugs if the built-in is needed within the method scope.♻️ Suggested rename
- all: bool = False, + fetch_all: bool = False, ) -> ClientTemplatesSimpleResponse: ... - skip_pagination=all, + skip_pagination=fetch_all,🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@app/operation/client_template.py` at line 122, The parameter name `all` shadows the Python built-in; rename it (for example to `include_all` or `fetch_all`) in the function signature where `all: bool = False` is declared, update the type hint and default, and replace all uses of `all` inside the function body and any callers to the new name (`include_all`/`fetch_all`) to avoid breaking references; also update any docstrings or tests that mention the parameter name.dashboard/src/service/api/index.ts (1)
2107-2124: Keep the client-template list responses on one pagination contract.
ClientTemplatesSimpleResponseistemplates + total, butClientTemplateResponseListswitches totemplates? + count. That makes the new pages special-case the same resource and increases the chance of wiring pagination or empty-state handling incorrectly. Since this file is generated, I'd align the OpenAPI response schemas and keeptemplatesrequired in both payloads.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@dashboard/src/service/api/index.ts` around lines 2107 - 2124, The two list response types diverge on pagination keys — ClientTemplatesSimpleResponse uses templates + total while ClientTemplateResponseList uses templates? + count — causing special-casing; update the contract so both use the same required fields: make ClientTemplateResponseList declare templates: ClientTemplateResponse[] (required) and total: number (replace count), and then update the OpenAPI schema or generator that produces this file so both response schemas align and templates remains required across generated outputs.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@app/db/crud/client_template.py`:
- Around line 225-244: The code only handles modified_template.is_default ==
True and ignores explicit False updates (so sending {"is_default": false} does
nothing); update the logic in the update block around modified_template,
db_template and ClientTemplate to handle when "is_default" is present and False:
either reject the request (raise a ValidationError/ValueError when template_data
contains "is_default" == False and db_template.is_default is True) or implement
reassignment by clearing db_template.is_default and atomically setting another
ClientTemplate of the same template_type to is_default=True (use db.execute with
update(ClientTemplate)... and ensure the operations run in the same transaction
before commit). Ensure you reference modified_template, db_template,
ClientTemplate and the existing db.execute(update(...)) usage so the change is
applied where template_data is checked and before commit/rollback.
- Around line 191-216: The create_client_template flow is racy:
count_client_templates_by_type + UPDATE + INSERT can allow two concurrent
creates to both treat themselves as the first/default. Fix by serializing
concurrent creators for a given template_type: inside create_client_template
open a transactional context and issue a SELECT ... FOR UPDATE (SQLAlchemy:
query/filter on ClientTemplate.template_type ==
client_template.template_type.value and call with_for_update()) to lock existing
rows for that template_type, then recount/compute is_first_for_type and run the
UPDATE and INSERT while the lock is held; alternatively add a DB-level partial
unique index (e.g. unique(template_type) WHERE is_system OR WHERE is_default)
and catch IntegrityError in create_client_template to retry the transaction once
— reference create_client_template, count_client_templates_by_type,
ClientTemplate, is_default, is_system.
In `@app/db/migrations/versions/e8c6a4f1d2b7_add_client_templates_table.py`:
- Around line 358-389: The migration is non-deterministic because
_template_content_or_default reads env vars and files at runtime; change
_template_content_or_default (and any similar logic used by upgrade()) to never
read environment variables or disk and instead always return the provided
default_content so the Alembic revision embeds a fixed payload; remove
CUSTOM_TEMPLATES_DIRECTORY and os.getenv usage from this migration file and
ensure upgrade() uses only static defaults, and relocate any env/file override
behavior into the runtime sync code (the service code that seeds
client_templates at application startup) rather than the migration.
- Around line 3-5: Update the migration docstring to match the actual parent
revision: change the "Revises: 20e2a5cf1e40" line in the module-level docstring
of e8c6a4f1d2b7_add_client_templates_table.py to "Revises: 2f3179c6dc49" so it
agrees with the down_revision variable set to "2f3179c6dc49".
In `@app/operation/client_template.py`:
- Around line 63-66: The validation for the parsed "list" field currently only
checks that _list is a non-empty list but doesn't ensure each element is a
string; update the check (in the same block where _list := parsed.get("list") is
validated) to iterate over _list and verify each item is an instance of str,
raising a ValueError if any element is not a string (include a clear message
indicating the offending index/type), and keep the existing non-empty check and
error messages consistent with the requirement that "list" be an array of
strings.
In `@app/routers/client_template.py`:
- Around line 56-63: The remove_client_template endpoint currently returns an
empty dict while using status.HTTP_204_NO_CONTENT, which creates a response body
and violates 204 semantics; update the handler (function remove_client_template)
to call client_template_operator.remove_client_template(db, template_id, admin)
as it does now but return a FastAPI/Starlette Response with the 204 status
(e.g., return Response(status_code=status.HTTP_204_NO_CONTENT)) instead of {} so
no body is sent; ensure imports include Response if missing and keep the
existing Depends (get_db, check_sudo_admin) and call to client_template_operator
unchanged.
In `@app/subscription/share.py`:
- Around line 48-52: The current branch incorrectly returns
ClashMetaConfiguration for both "clash" and "clash_meta"; restore the distinct
renderer by returning ClashConfiguration when config_format == "clash" and only
returning ClashMetaConfiguration for config_format == "clash_meta". Update the
conditional handling around config_format to call ClashConfiguration with
clash_template_content=client_templates["CLASH_SUBSCRIPTION_TEMPLATE"] and
common_kwargs for the "clash" case, and keep the existing ClashMetaConfiguration
return for the "clash_meta" case so each format uses its intended class.
In `@dashboard/src/components/common/mobile-yaml-ace-editor.tsx`:
- Around line 3-4: The file mobile-yaml-ace-editor.tsx imports the wrong light
theme module; update the imports so the light theme actually loaded matches the
theme selected ('textmate'). Replace the import of
'ace-builds/src-noconflict/theme-tomorrow_night' with
'ace-builds/src-noconflict/theme-textmate' so the editor's selected light theme
('textmate') can be found at runtime (adjust the import statement near the top
of mobile-yaml-ace-editor.tsx).
In `@dashboard/src/components/dialogs/client-template-modal.tsx`:
- Around line 219-233: The content FormField render for the "content" field
currently only shows a custom validation UI (validation.isValid /
validation.error) and never surfaces the FormField's fieldState.error, so
untouched/invalid templates show no feedback; update the render for FormField
name="content" (the render function that calls renderEditor(field) inside
FormItem/FormControl) to also render the form library's error/FormMessage (e.g.,
fieldState.error or <FormMessage>) for both desktop and mobile layouts—ensure
you use the FormField render prop's fieldState to display fieldState.error
alongside or instead of the custom validation UI (and apply the same change in
the other instance referenced around lines 295-309) so all resolver errors are
visible.
In `@dashboard/src/components/templates/client-template-actions-menu.tsx`:
- Around line 39-47: The current use of dangerouslySetInnerHTML with
t('clientTemplates.deletePrompt', ...) injecting template.name risks XSS;
replace this by removing dangerouslySetInnerHTML and render the message as React
nodes (e.g., use react-i18next's <Trans> or call t with a non-HTML default and
compose JSX) so you can render the bold part as <b>{template.name}</b>, or if
you must keep HTML use a sanitizer (e.g., DOMPurify) to sanitize template.name
before inserting; update the span usage around dir and remove
dangerouslySetInnerHTML, referencing the t call and template.name for the new
safe rendering approach.
In `@dashboard/src/pages/_dashboard.templates.client.tsx`:
- Around line 55-60: The filter callback in the useMemo for filteredTemplates
shadows the translation function t from useTranslation(); rename the callback
parameter (e.g., from t to template or tmpl) in the templates.filter((t:
ClientTemplateResponse) => ...) so it no longer conflicts, update any references
inside the callback to the new name, and keep the existing types
(ClientTemplateResponse), searchQuery logic and the dependency array ([data,
searchQuery]) unchanged.
In `@dashboard/src/pages/_dashboard.templates.user.tsx`:
- Around line 50-54: The form reset is discarding zero values because data_limit
and expire_duration use the || undefined pattern; update the hydration in the
form.reset call in _dashboard.templates.user.tsx to use the same nullish/numeric
check as on_hold_timeout (e.g., use the nullish coalescing or explicit null
check for userTemplate.data_limit and userTemplate.expire_duration so 0 is
preserved and only null/undefined becomes undefined) so zero-valued fields
remain when editing.
In `@tests/api/helpers.py`:
- Around line 84-85: The default content string used in tests/api/helpers.py is
malformed JSON and should be replaced with a valid, well-formed JSON template so
create_client_template() and callers don't break; update the variable named
content (and any default is_default behavior) to a complete JSON string that
closes all objects/arrays and includes the expected keys (outbounds, inbounds
with a vmess client object containing id/alterId/email) so
create_client_template() can parse/modify it reliably.
---
Nitpick comments:
In `@app/operation/client_template.py`:
- Line 122: The parameter name `all` shadows the Python built-in; rename it (for
example to `include_all` or `fetch_all`) in the function signature where `all:
bool = False` is declared, update the type hint and default, and replace all
uses of `all` inside the function body and any callers to the new name
(`include_all`/`fetch_all`) to avoid breaking references; also update any
docstrings or tests that mention the parameter name.
In `@dashboard/src/components/templates/client-template-actions-menu.tsx`:
- Around line 151-161: The onSelect handler for DropdownMenuItem is passing a
React.SyntheticEvent but handleDeleteClick is typed to accept a DOM Event;
update the types to match by changing handleDeleteClick's parameter type to
React.SyntheticEvent (or Event | React.SyntheticEvent) so signatures align, then
call e.stopPropagation() and pass e to handleDeleteClick; alternatively inline
the delete logic in the onSelect callback and remove the mismatched
handleDeleteClick parameter to avoid the type mismatch (referencing
DropdownMenuItem, onSelect, and handleDeleteClick).
In `@dashboard/src/service/api/index.ts`:
- Around line 2107-2124: The two list response types diverge on pagination keys
— ClientTemplatesSimpleResponse uses templates + total while
ClientTemplateResponseList uses templates? + count — causing special-casing;
update the contract so both use the same required fields: make
ClientTemplateResponseList declare templates: ClientTemplateResponse[]
(required) and total: number (replace count), and then update the OpenAPI schema
or generator that produces this file so both response schemas align and
templates remains required across generated outputs.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: de138aaf-3809-4cee-acda-728df29c0a57
📒 Files selected for processing (34)
app/app_factory.pyapp/db/crud/__init__.pyapp/db/crud/client_template.pyapp/db/migrations/versions/e8c6a4f1d2b7_add_client_templates_table.pyapp/db/models.pyapp/models/client_template.pyapp/nats/message.pyapp/operation/__init__.pyapp/operation/client_template.pyapp/routers/__init__.pyapp/routers/client_template.pyapp/subscription/client_templates.pyapp/subscription/links.pyapp/subscription/share.pyapp/subscription/xray.pydashboard/public/statics/locales/en.jsondashboard/public/statics/locales/fa.jsondashboard/public/statics/locales/ru.jsondashboard/public/statics/locales/zh.jsondashboard/src/components/common/mobile-yaml-ace-editor.tsxdashboard/src/components/dialogs/client-template-modal.tsxdashboard/src/components/forms/client-template-form.tsdashboard/src/components/layout/sidebar.tsxdashboard/src/components/templates/client-template-actions-menu.tsxdashboard/src/components/templates/client-template.tsxdashboard/src/components/templates/use-client-templates-list-columns.tsxdashboard/src/pages/_dashboard.templates.client.tsxdashboard/src/pages/_dashboard.templates.tsxdashboard/src/pages/_dashboard.templates.user.tsxdashboard/src/router.tsxdashboard/src/service/api/index.tstests/api/conftest.pytests/api/helpers.pytests/api/test_client_template.py
✅ Files skipped from review due to trivial changes (2)
- app/nats/message.py
- app/db/crud/init.py
🚧 Files skipped from review as they are similar to previous changes (6)
- app/routers/init.py
- app/app_factory.py
- app/subscription/links.py
- app/operation/init.py
- app/subscription/xray.py
- app/db/models.py
| async def create_client_template(db: AsyncSession, client_template: ClientTemplateCreate) -> ClientTemplate: | ||
| type_count = await count_client_templates_by_type(db, client_template.template_type) | ||
| is_first_for_type = type_count == 0 | ||
| should_be_default = client_template.is_default or is_first_for_type | ||
|
|
||
| if should_be_default: | ||
| await db.execute( | ||
| update(ClientTemplate) | ||
| .where(ClientTemplate.template_type == client_template.template_type.value) | ||
| .values(is_default=False) | ||
| ) | ||
|
|
||
| db_template = ClientTemplate( | ||
| name=client_template.name, | ||
| template_type=client_template.template_type.value, | ||
| content=client_template.content, | ||
| is_default=should_be_default, | ||
| is_system=is_first_for_type, | ||
| ) | ||
| db.add(db_template) | ||
| try: | ||
| await db.commit() | ||
| except IntegrityError: | ||
| await db.rollback() | ||
| raise | ||
| await db.refresh(db_template) |
There was a problem hiding this comment.
The first/default invariants are still racy on create.
count_client_templates_by_type() + UPDATE + INSERT is a TOCTOU sequence. Two concurrent creates for the same template_type can both observe type_count == 0 and insert rows with is_system=True / is_default=True, which breaks the delete/default rules built around a single first template.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@app/db/crud/client_template.py` around lines 191 - 216, The
create_client_template flow is racy: count_client_templates_by_type + UPDATE +
INSERT can allow two concurrent creates to both treat themselves as the
first/default. Fix by serializing concurrent creators for a given template_type:
inside create_client_template open a transactional context and issue a SELECT
... FOR UPDATE (SQLAlchemy: query/filter on ClientTemplate.template_type ==
client_template.template_type.value and call with_for_update()) to lock existing
rows for that template_type, then recount/compute is_first_for_type and run the
UPDATE and INSERT while the lock is held; alternatively add a DB-level partial
unique index (e.g. unique(template_type) WHERE is_system OR WHERE is_default)
and catch IntegrityError in create_client_template to retry the transaction once
— reference create_client_template, count_client_templates_by_type,
ClientTemplate, is_default, is_system.
| template_data = modified_template.model_dump(exclude_none=True) | ||
|
|
||
| if modified_template.is_default is True: | ||
| await db.execute( | ||
| update(ClientTemplate) | ||
| .where(ClientTemplate.template_type == db_template.template_type) | ||
| .values(is_default=False) | ||
| ) | ||
| db_template.is_default = True | ||
|
|
||
| if "name" in template_data: | ||
| db_template.name = template_data["name"] | ||
| if "content" in template_data: | ||
| db_template.content = template_data["content"] | ||
|
|
||
| try: | ||
| await db.commit() | ||
| except IntegrityError: | ||
| await db.rollback() | ||
| raise |
There was a problem hiding this comment.
is_default=False is accepted but never applied.
model_dump(exclude_none=True) preserves False, but this method only handles is_default is True. A caller can send {"is_default": false} and get a successful update even though the stored flag never changes. Either reject that state explicitly or implement reassignment to another default.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@app/db/crud/client_template.py` around lines 225 - 244, The code only handles
modified_template.is_default == True and ignores explicit False updates (so
sending {"is_default": false} does nothing); update the logic in the update
block around modified_template, db_template and ClientTemplate to handle when
"is_default" is present and False: either reject the request (raise a
ValidationError/ValueError when template_data contains "is_default" == False and
db_template.is_default is True) or implement reassignment by clearing
db_template.is_default and atomically setting another ClientTemplate of the same
template_type to is_default=True (use db.execute with update(ClientTemplate)...
and ensure the operations run in the same transaction before commit). Ensure you
reference modified_template, db_template, ClientTemplate and the existing
db.execute(update(...)) usage so the change is applied where template_data is
checked and before commit/rollback.
| Revision ID: e8c6a4f1d2b7 | ||
| Revises: 20e2a5cf1e40 | ||
| Create Date: 2026-02-20 15:45:00.000000 |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
set -euo pipefail
rg -n --type=py '^(revision|down_revision)\s*=' app/db/migrations/versions
rg -n --type=py '20e2a5cf1e40|2f3179c6dc49|e8c6a4f1d2b7' app/db/migrations/versionsRepository: PasarGuard/panel
Length of output: 25057
Fix the migration parent revision in the docstring.
Line 4 contains a stale docstring that claims this migration revises 20e2a5cf1e40, but line 18 correctly sets down_revision to 2f3179c6dc49. The docstring and down_revision must agree. Update line 4 from Revises: 20e2a5cf1e40 to Revises: 2f3179c6dc49.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@app/db/migrations/versions/e8c6a4f1d2b7_add_client_templates_table.py` around
lines 3 - 5, Update the migration docstring to match the actual parent revision:
change the "Revises: 20e2a5cf1e40" line in the module-level docstring of
e8c6a4f1d2b7_add_client_templates_table.py to "Revises: 2f3179c6dc49" so it
agrees with the down_revision variable set to "2f3179c6dc49".
| def _template_content_or_default( | ||
| env_key: str, | ||
| path_from_project_root: str, | ||
| default_content: str, | ||
| ) -> str: | ||
| env_value = os.getenv(env_key) | ||
| if env_value: | ||
| return env_value | ||
|
|
||
| custom_templates_directory = os.getenv("CUSTOM_TEMPLATES_DIRECTORY") | ||
| if custom_templates_directory: | ||
| custom_base_path = Path(custom_templates_directory).expanduser() | ||
| project_relative_path = Path(path_from_project_root) | ||
| try: | ||
| custom_relative_path = project_relative_path.relative_to("app/templates") | ||
| except ValueError: | ||
| custom_relative_path = project_relative_path | ||
|
|
||
| custom_file_path = custom_base_path / custom_relative_path | ||
| try: | ||
| if custom_file_path.exists(): | ||
| return custom_file_path.read_text(encoding="utf-8") | ||
| except OSError: | ||
| pass | ||
|
|
||
| file_path = PROJECT_ROOT / path_from_project_root | ||
| try: | ||
| if file_path.exists(): | ||
| return file_path.read_text(encoding="utf-8") | ||
| except OSError: | ||
| pass | ||
| return default_content |
There was a problem hiding this comment.
Keep this migration deterministic.
upgrade() currently reads env vars and whatever template files exist on disk at execution time, so the same Alembic revision can seed different client_templates data across environments or even across fresh installs months later. Migrations should freeze their payload; move env/file override behavior to runtime sync code instead.
Also applies to: 406-430
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@app/db/migrations/versions/e8c6a4f1d2b7_add_client_templates_table.py` around
lines 358 - 389, The migration is non-deterministic because
_template_content_or_default reads env vars and files at runtime; change
_template_content_or_default (and any similar logic used by upgrade()) to never
read environment variables or disk and instead always return the provided
default_content so the Alembic revision embeds a fixed payload; remove
CUSTOM_TEMPLATES_DIRECTORY and os.getenv usage from this migration file and
ensure upgrade() uses only static defaults, and relocate any env/file override
behavior into the runtime sync code (the service code that seeds
client_templates at application startup) rather than the migration.
| if (_list := parsed.get("list")) is None or not isinstance(_list, list): | ||
| raise ValueError("User-Agent template content must contain a 'list' field with an array of strings") | ||
| if not _list: | ||
| raise ValueError("User-Agent template content must contain at least one User-Agent string") |
There was a problem hiding this comment.
User-Agent list items are not validated as strings.
The error message states the list field should contain "an array of strings", but the validation only checks that list exists, is a list, and is non-empty. Individual elements are not validated as strings, allowing arrays of other types (integers, objects, etc.) to pass.
🛡️ Proposed fix to validate list items
if (_list := parsed.get("list")) is None or not isinstance(_list, list):
raise ValueError("User-Agent template content must contain a 'list' field with an array of strings")
if not _list:
raise ValueError("User-Agent template content must contain at least one User-Agent string")
+ if not all(isinstance(item, str) for item in _list):
+ raise ValueError("All User-Agent list items must be strings")📝 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.
| if (_list := parsed.get("list")) is None or not isinstance(_list, list): | |
| raise ValueError("User-Agent template content must contain a 'list' field with an array of strings") | |
| if not _list: | |
| raise ValueError("User-Agent template content must contain at least one User-Agent string") | |
| if (_list := parsed.get("list")) is None or not isinstance(_list, list): | |
| raise ValueError("User-Agent template content must contain a 'list' field with an array of strings") | |
| if not _list: | |
| raise ValueError("User-Agent template content must contain at least one User-Agent string") | |
| if not all(isinstance(item, str) for item in _list): | |
| raise ValueError("All User-Agent list items must be strings") |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@app/operation/client_template.py` around lines 63 - 66, The validation for
the parsed "list" field currently only checks that _list is a non-empty list but
doesn't ensure each element is a string; update the check (in the same block
where _list := parsed.get("list") is validated) to iterate over _list and verify
each item is an instance of str, raising a ValueError if any element is not a
string (include a clear message indicating the offending index/type), and keep
the existing non-empty check and error messages consistent with the requirement
that "list" be an array of strings.
| <FormField | ||
| control={form.control} | ||
| name="content" | ||
| render={({ field }) => ( | ||
| <FormItem className="h-full"> | ||
| <FormControl className="h-full"> | ||
| <div className="h-full" dir="ltr">{renderEditor(field)}</div> | ||
| </FormControl> | ||
| {!validation.isValid && validation.error && ( | ||
| <div className="absolute bottom-2 left-2 right-2 rounded border border-destructive/30 bg-destructive/10 px-2 py-1 text-xs text-destructive"> | ||
| {validation.error} | ||
| </div> | ||
| )} | ||
| </FormItem> | ||
| )} |
There was a problem hiding this comment.
Surface content resolver errors in both editor layouts.
The content field never renders fieldState.error, so an untouched empty template can fail validation with no visible feedback. Desktop has no FormMessage at all, and mobile only shows the custom JSON/YAML error state.
💡 Suggested change
- <FormField
- control={form.control}
- name="content"
- render={({ field }) => (
+ <FormField
+ control={form.control}
+ name="content"
+ render={({ field, fieldState }) => (
<FormItem className="h-full">
<FormControl className="h-full">
<div className="h-full" dir="ltr">{renderEditor(field)}</div>
</FormControl>
+ {fieldState.error && <FormMessage>{fieldState.error.message}</FormMessage>}
{!validation.isValid && validation.error && (
<div className="absolute bottom-2 left-2 right-2 rounded border border-destructive/30 bg-destructive/10 px-2 py-1 text-xs text-destructive">
{validation.error}
</div>
)}
</FormItem>
)}
/>
@@
- <FormField
- control={form.control}
- name="content"
- render={({ field }) => (
+ <FormField
+ control={form.control}
+ name="content"
+ render={({ field, fieldState }) => (
<FormItem className="flex flex-col">
@@
- {!validation.isValid && validation.error && <FormMessage>{validation.error}</FormMessage>}
+ {fieldState.error && <FormMessage>{fieldState.error.message}</FormMessage>}
+ {!fieldState.error && !validation.isValid && validation.error && <FormMessage>{validation.error}</FormMessage>}
</FormItem>
)}
/>Also applies to: 295-309
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@dashboard/src/components/dialogs/client-template-modal.tsx` around lines 219
- 233, The content FormField render for the "content" field currently only shows
a custom validation UI (validation.isValid / validation.error) and never
surfaces the FormField's fieldState.error, so untouched/invalid templates show
no feedback; update the render for FormField name="content" (the render function
that calls renderEditor(field) inside FormItem/FormControl) to also render the
form library's error/FormMessage (e.g., fieldState.error or <FormMessage>) for
both desktop and mobile layouts—ensure you use the FormField render prop's
fieldState to display fieldState.error alongside or instead of the custom
validation UI (and apply the same change in the other instance referenced around
lines 295-309) so all resolver errors are visible.
| <span | ||
| dir={dir} | ||
| dangerouslySetInnerHTML={{ | ||
| __html: t('clientTemplates.deletePrompt', { | ||
| name: template.name, | ||
| defaultValue: `Are you sure you want to delete <b>{{name}}</b>? This action cannot be undone.`, | ||
| }), | ||
| }} | ||
| /> |
There was a problem hiding this comment.
Potential XSS via unsanitized template name in dangerouslySetInnerHTML.
The static analysis tool correctly flagged this. While template.name originates from the database and is likely validated on input, using dangerouslySetInnerHTML with user-controlled content poses an XSS risk if the backend validation is ever bypassed or relaxed.
Consider sanitizing the name or using a safer approach:
Proposed safer alternative
<AlertDialogDescription>
- <span
- dir={dir}
- dangerouslySetInnerHTML={{
- __html: t('clientTemplates.deletePrompt', {
- name: template.name,
- defaultValue: `Are you sure you want to delete <b>{{name}}</b>? This action cannot be undone.`,
- }),
- }}
- />
+ <span dir={dir}>
+ {t('clientTemplates.deletePromptPrefix', { defaultValue: 'Are you sure you want to delete ' })}
+ <b>{template.name}</b>
+ {t('clientTemplates.deletePromptSuffix', { defaultValue: '? This action cannot be undone.' })}
+ </span>
</AlertDialogDescription>Alternatively, if the dangerouslySetInnerHTML pattern is intentional for i18n flexibility across the codebase, ensure the backend strictly validates and sanitizes template names to prevent HTML injection.
🧰 Tools
🪛 ast-grep (0.41.1)
[warning] 40-40: Usage of dangerouslySetInnerHTML detected. This bypasses React's built-in XSS protection. Always sanitize HTML content using libraries like DOMPurify before injecting it into the DOM to prevent XSS attacks.
Context: dangerouslySetInnerHTML
Note: [CWE-79] Improper Neutralization of Input During Web Page Generation [REFERENCES]
- https://reactjs.org/docs/dom-elements.html#dangerouslysetinnerhtml
- https://cwe.mitre.org/data/definitions/79.html
(react-unsafe-html-injection)
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@dashboard/src/components/templates/client-template-actions-menu.tsx` around
lines 39 - 47, The current use of dangerouslySetInnerHTML with
t('clientTemplates.deletePrompt', ...) injecting template.name risks XSS;
replace this by removing dangerouslySetInnerHTML and render the message as React
nodes (e.g., use react-i18next's <Trans> or call t with a non-HTML default and
compose JSX) so you can render the bold part as <b>{template.name}</b>, or if
you must keep HTML use a sanitizer (e.g., DOMPurify) to sanitize template.name
before inserting; update the span usage around dir and remove
dangerouslySetInnerHTML, referencing the t call and template.name for the new
safe rendering approach.
| const filteredTemplates = useMemo(() => { | ||
| const templates = data?.templates || [] | ||
| if (!searchQuery.trim()) return templates | ||
| const query = searchQuery.toLowerCase().trim() | ||
| return templates.filter((t: ClientTemplateResponse) => t.name?.toLowerCase().includes(query) || t.template_type?.toLowerCase().includes(query)) | ||
| }, [data, searchQuery]) |
There was a problem hiding this comment.
Avoid shadowing the t translation function.
The filter callback parameter t shadows the t function from useTranslation(), which could cause confusion and potential bugs if the translation function is needed within the filter.
Proposed fix
const filteredTemplates = useMemo(() => {
const templates = data?.templates || []
if (!searchQuery.trim()) return templates
const query = searchQuery.toLowerCase().trim()
- return templates.filter((t: ClientTemplateResponse) => t.name?.toLowerCase().includes(query) || t.template_type?.toLowerCase().includes(query))
+ return templates.filter((template: ClientTemplateResponse) => template.name?.toLowerCase().includes(query) || template.template_type?.toLowerCase().includes(query))
}, [data, searchQuery])📝 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.
| const filteredTemplates = useMemo(() => { | |
| const templates = data?.templates || [] | |
| if (!searchQuery.trim()) return templates | |
| const query = searchQuery.toLowerCase().trim() | |
| return templates.filter((t: ClientTemplateResponse) => t.name?.toLowerCase().includes(query) || t.template_type?.toLowerCase().includes(query)) | |
| }, [data, searchQuery]) | |
| const filteredTemplates = useMemo(() => { | |
| const templates = data?.templates || [] | |
| if (!searchQuery.trim()) return templates | |
| const query = searchQuery.toLowerCase().trim() | |
| return templates.filter((template: ClientTemplateResponse) => template.name?.toLowerCase().includes(query) || template.template_type?.toLowerCase().includes(query)) | |
| }, [data, searchQuery]) |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@dashboard/src/pages/_dashboard.templates.client.tsx` around lines 55 - 60,
The filter callback in the useMemo for filteredTemplates shadows the translation
function t from useTranslation(); rename the callback parameter (e.g., from t to
template or tmpl) in the templates.filter((t: ClientTemplateResponse) => ...) so
it no longer conflicts, update any references inside the callback to the new
name, and keep the existing types (ClientTemplateResponse), searchQuery logic
and the dependency array ([data, searchQuery]) unchanged.
| form.reset({ | ||
| name: userTemplate.name || undefined, | ||
| status: userTemplate.status || undefined, | ||
| data_limit: userTemplate.data_limit || undefined, | ||
| expire_duration: userTemplate.expire_duration || undefined, |
There was a problem hiding this comment.
Preserve zero-valued fields when hydrating the edit form.
data_limit and expire_duration are reset with || undefined, so a stored 0 comes back as blank and gets resubmitted differently after an edit. Use the same numeric/nullish check pattern you already use for on_hold_timeout.
💡 Suggested change
- data_limit: userTemplate.data_limit || undefined,
- expire_duration: userTemplate.expire_duration || undefined,
+ data_limit: typeof userTemplate.data_limit === 'number' ? userTemplate.data_limit : undefined,
+ expire_duration: typeof userTemplate.expire_duration === 'number' ? userTemplate.expire_duration : undefined,📝 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.
| form.reset({ | |
| name: userTemplate.name || undefined, | |
| status: userTemplate.status || undefined, | |
| data_limit: userTemplate.data_limit || undefined, | |
| expire_duration: userTemplate.expire_duration || undefined, | |
| form.reset({ | |
| name: userTemplate.name || undefined, | |
| status: userTemplate.status || undefined, | |
| data_limit: typeof userTemplate.data_limit === 'number' ? userTemplate.data_limit : undefined, | |
| expire_duration: typeof userTemplate.expire_duration === 'number' ? userTemplate.expire_duration : undefined, |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@dashboard/src/pages/_dashboard.templates.user.tsx` around lines 50 - 54, The
form reset is discarding zero values because data_limit and expire_duration use
the || undefined pattern; update the hydration in the form.reset call in
_dashboard.templates.user.tsx to use the same nullish/numeric check as
on_hold_timeout (e.g., use the nullish coalescing or explicit null check for
userTemplate.data_limit and userTemplate.expire_duration so 0 is preserved and
only null/undefined becomes undefined) so zero-valued fields remain when
editing.
| content: str = '{"outbounds": [{"tag":"direct","protocol":"freedom","settings":{}}],"inbounds":[{"tag":"proxy","protocol":"vmess","settings":{"clients":[{"id":"uuid","alterId":0,"email":"}', | ||
| is_default: bool = False, |
There was a problem hiding this comment.
Use valid default template content in test helper.
The default content on Line 84 is malformed JSON (truncated/unterminated). This makes create_client_template() brittle and can fail tests that rely on defaults.
💡 Proposed fix
- content: str = '{"outbounds": [{"tag":"direct","protocol":"freedom","settings":{}}],"inbounds":[{"tag":"proxy","protocol":"vmess","settings":{"clients":[{"id":"uuid","alterId":0,"email":"}',
+ content: str = '{"outbounds":[{"tag":"direct","protocol":"freedom","settings":{}}],"inbounds":[]}',📝 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.
| content: str = '{"outbounds": [{"tag":"direct","protocol":"freedom","settings":{}}],"inbounds":[{"tag":"proxy","protocol":"vmess","settings":{"clients":[{"id":"uuid","alterId":0,"email":"}', | |
| is_default: bool = False, | |
| content: str = '{"outbounds":[{"tag":"direct","protocol":"freedom","settings":{}}],"inbounds":[]}', | |
| is_default: bool = False, |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@tests/api/helpers.py` around lines 84 - 85, The default content string used
in tests/api/helpers.py is malformed JSON and should be replaced with a valid,
well-formed JSON template so create_client_template() and callers don't break;
update the variable named content (and any default is_default behavior) to a
complete JSON string that closes all objects/arrays and includes the expected
keys (outbounds, inbounds with a vmess client object containing
id/alterId/email) so create_client_template() can parse/modify it reliably.
1 participant
Summary by CodeRabbit
New Features
Documentation
Tests