Conversation
- Add HWID settings page with device registration policy configuration - Implement hardware ID management modal for per-user HWID viewing and deletion - Add HWID limit fields to user template and subscription forms - Add comprehensive localization for HWID features across en, fa, ru, zh locales - Add user-hwids-modal component for managing registered device IDs - Add dedicated HWID settings route (_dashboard.settings.hwid.tsx) - Update sidebar navigation to include HWID settings link - Add HWID-related API endpoints to service layer - Support device limit configuration with fallback, minimum, and maximum bounds - Enable per-user HWID limit overrides with validation
|
Important Review skippedAuto reviews are disabled on base/target branches other than the default branch. Please check the settings in the CodeRabbit UI or the ⚙️ Run configurationConfiguration used: Organization UI Review profile: CHILL Plan: Pro Run ID: You can disable this status message by setting the Use the checkbox below for a quick retry:
WalkthroughThis PR introduces a comprehensive hardware ID (HWID) tracking system enabling device registration and per-user device limits. The implementation spans database models, CRUD operations, admin APIs, subscription tracking integration, and full dashboard UI with multi-language support and configuration settings. ChangesHWID Management System
Sequence Diagram(s)sequenceDiagram
participant Client
participant SubRouter
participant SubOp
participant HWIDOp
participant DB
Client->>SubRouter: GET /sub/user_agent (X-HWID: device123, X-Device-OS: ios)
SubRouter->>SubOp: user_subscription(..., x_hwid=device123, x_device_os=ios)
SubOp->>SubOp: validate_and_register_hwid
alt HWID enabled in settings
alt existing HWID found
SubOp->>HWIDOp: update_hwid_last_used(hwid_obj)
else new HWID
SubOp->>SubOp: check user limit (fallback_limit=3)
alt limit exceeded
SubOp-->>SubRouter: raise 403 Device limit reached
else limit ok
SubOp->>HWIDOp: register_user_hwid(user_id, device123, device_os=ios)
end
end
end
SubOp->>DB: user_sub_update(..., hwid=device123)
SubRouter-->>Client: subscription config
Estimated code review effort🎯 4 (Complex) | ⏱️ ~60 minutes Possibly related PRs
Suggested labels
Poem
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
Tip 💬 Introducing Slack Agent: The best way for teams to turn conversations into code.Slack Agent is built on CodeRabbit's deep understanding of your code, so your team can collaborate across the entire SDLC without losing context.
Built for teams:
One agent for your entire SDLC. Right inside Slack. 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 |
Contributor
Author
|
@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: 11
🧹 Nitpick comments (3)
dashboard/src/features/templates/dialogs/user-template-modal.tsx (1)
179-184: 💤 Low valueConsider simplifying redundant null check.
Line 179 already normalizes
values.hwid_limitto eithernullor a number, so the null check on line 184 is redundant. The code is correct but could be more concise.♻️ Simplified version
-const normalizedHwidLimit = values.hwid_limit == null ? null : Number(values.hwid_limit) +const normalizedHwidLimit = values.hwid_limit == null ? null : Number(values.hwid_limit) // Build payload according to UserTemplateCreate interface const submitData = { name: values.name, data_limit: hasDataLimit ? gbToBytes(normalizedDataLimitGb as any) : 0, - hwid_limit: normalizedHwidLimit == null ? null : Number.isFinite(normalizedHwidLimit) ? Math.floor(normalizedHwidLimit) : null, + hwid_limit: normalizedHwidLimit === null || !Number.isFinite(normalizedHwidLimit) ? null : Math.floor(normalizedHwidLimit),🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@dashboard/src/features/templates/dialogs/user-template-modal.tsx` around lines 179 - 184, The hwid_limit assignment in the submitData object is doing a redundant null check because normalizedHwidLimit has already been normalized to null or a number; simplify the expression in the submitData construction (in user-template-modal.tsx) to directly test Number.isFinite(normalizedHwidLimit) ? Math.floor(normalizedHwidLimit) : null (or equivalent) so the code uses the existing normalizedHwidLimit variable and removes the extra "== null" branch while preserving the current semantics.dashboard/src/components/layout/sidebar.tsx (1)
257-261: 💤 Low valueAdd trailing comma for consistency.
The closing brace should have a trailing comma to match the style of all other items in the
itemsarray.📝 Proposed fix
{ title: 'settings.hwid.title', url: '/settings/hwid', icon: Fingerprint - }, + },🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@dashboard/src/components/layout/sidebar.tsx` around lines 257 - 261, The object entry with title 'settings.hwid.title' in the items array (the object containing url '/settings/hwid' and icon Fingerprint) is missing a trailing comma; update that object literal to include a trailing comma after its closing brace so it matches the other entries in the items array and the project's consistent comma style.app/models/settings.py (1)
288-294: ⚡ Quick winAdd cross-field validation for HWID limits.
HWIDSettingscurrently validates each field independently, so inconsistent combinations can still pass (e.g.,min_limit > max_limitor fallback outside bounds). Add a model-level invariant check to reject invalid configurations early.♻️ Proposed fix
class HWIDSettings(BaseModel): enabled: bool = Field(default=False) forced: bool = Field(default=False) fallback_limit: int = Field(default=0, ge=0) min_limit: int = Field(default=0, ge=0) max_limit: int = Field(default=0, ge=0) + + `@model_validator`(mode="after") + def validate_limit_bounds(self): + if self.min_limit > self.max_limit: + raise ValueError("min_limit cannot be greater than max_limit") + if not (self.min_limit <= self.fallback_limit <= self.max_limit): + raise ValueError("fallback_limit must be between min_limit and max_limit") + return self🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@app/models/settings.py` around lines 288 - 294, HWIDSettings currently only validates fields individually, so invalid combos like min_limit > max_limit or fallback_limit outside [min_limit, max_limit] can slip through; add a model-level validator (e.g., a Pydantic `@root_validator` or model validator) on class HWIDSettings that checks invariants: ensure min_limit <= max_limit, ensure fallback_limit is between min_limit and max_limit (or equal to a defined sentinel behavior if disabled), and raise a ValueError with a clear message if any invariant fails; implement this in the HWIDSettings class so invalid configurations are rejected at model creation.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@app/db/crud/hwid.py`:
- Around line 44-47: Wrap the db.add(new_hwid) / await db.commit() / await
db.refresh(new_hwid) block in a try/except that catches
sqlalchemy.exc.IntegrityError, call await db.rollback() inside the except, then
query the DB for the existing record using the same unique key(s) (e.g. by
hardware_id or whatever unique column the model uses) and return that existing
instance instead of raising; ensure IntegrityError is imported and that you only
suppress duplicates (re-raise other DB errors). Keep references to the current
variables/new_hwid and use the model's unique lookup (e.g. get_by_hardware_id or
a query on the model) to return the already-registered object.
In `@app/db/crud/user.py`:
- Around line 969-970: The modify logic for hwid_limit only assigns when
modify.hwid_limit is not None, preventing clients from clearing an existing
db_user.hwid_limit back to NULL; update the user-modify handler (where
modify.hwid_limit and db_user.hwid_limit are referenced) to distinguish "no
change" from "explicit clear" by adding an explicit flag or using a sentinel
(e.g., a separate boolean on the request or an Optional wrapper) so that when
the client signals clear you set db_user.hwid_limit = None, otherwise leave it
unchanged; locate the assignment involving modify.hwid_limit and replace the
conditional to apply the clear behavior when the explicit-clear indicator is
present.
In `@app/operation/subscription.py`:
- Around line 280-289: The HWID limit check is non-atomic (uses
get_user_hwid_count then register_user_hwid) and can be bypassed by concurrent
requests; fix by performing the check+insert inside a DB transaction or by
moving the enforcement into register_user_hwid so it does the count-and-insert
atomically (e.g., start a transaction, SELECT ... FOR UPDATE on the user's row
or use a DB constraint and attempt the insert, then raise the same 403 error if
the count >= limit). Update the code around get_user_hwid_count and
register_user_hwid to use this transactional/atomic approach so concurrent
requests cannot exceed the limit.
In `@app/routers/dependencies/_common.py`:
- Around line 96-100: The code currently only wraps non-required defaults in
Header(...) so required parameters (default == Parameter.empty) remain unbound
and become query params; update the logic around default/field_info/field_name
in the block that checks isinstance(default, (HeaderParam, ParameterOverride))
to also wrap required parameters by calling Header(..., alias=alias) when
default is Parameter.empty (use the same alias calculation using
field_info.alias or field_name.replace("_", "-").title()), so both required and
optional header fields are explicitly bound as Header with the correct alias.
In `@dashboard/public/statics/locales/ru.json`:
- Around line 1052-1053: Replace the inconsistent placeholder text "Резерв, 0 =
без лимита" with the same phrasing used in the template ("По умолчанию, 0 = без
лимита") for the key userDialog.hwidLimitPlaceholder and the other occurrences
flagged (the entries around lines 2401-2402); update the JSON values for
userDialog.hwidLimitPlaceholder (and the matching keys at the other location) to
"По умолчанию, 0 = без лимита" so all HWID limit placeholders use identical
wording.
In `@dashboard/public/statics/locales/zh.json`:
- Around line 1406-1407: Update the HWID placeholder translation to match the
template wording: replace the value of "userDialog.hwidLimitPlaceholder" (and
the duplicate entry at the other occurrence) from the current phrase using "备用"
to use "默认，0 = 无限制" so the placeholder text is consistent with the template copy
referenced by the key "userDialog.hwidLimitPlaceholder".
In `@dashboard/src/features/users/dialogs/user-hwids-modal.tsx`:
- Around line 139-141: The icon-only remove Button in user-hwids-modal.tsx lacks
an accessible name; update the Button (the element rendering <Trash2 /> and
calling setHwidToDelete(item.hwid)) to provide one—either add an explicit
aria-label (e.g., aria-label={`Remove HWID ${item.hwid}`} or aria-label="Remove
hardware ID") or include visually hidden text (a <span
className="sr-only">Remove hardware ID</span>) inside the Button so screen
readers can identify the action.
In `@dashboard/src/features/users/forms/user-form.ts`:
- Line 54: The hwid_limit schema currently allows fractional numbers; update the
validation for hwid_limit in user-form.ts to require non-negative integers by
adding integer enforcement (use the z.number().int().min(0) form) while
preserving nullable() and optional() so the field can still be null/omitted;
update the hwid_limit definition to use the integer-constrained validator to
prevent fractional device counts.
In `@dashboard/src/pages/_dashboard.settings.hwid.tsx`:
- Around line 79-82: The cancel handler currently shows a success toast which is
non-standard; update the handleCancel function so it no longer calls
toast.success(...) — either remove the toast call entirely or replace it with a
neutral/info toast (e.g., toast.info) and/or a less-assertive message key
(t('settings.hwid.cancelSuccess', ...) can be replaced or removed); keep the
existing form.reset(formValues) behavior and adjust the toast usage accordingly
in handleCancel to avoid showing a success state for a cancel action.
In `@dashboard/src/service/api/index.ts`:
- Around line 11577-11579: The URL path parameters are interpolated without
encoding in userSubscriptionRaw (token) and the similar hwid-based fetcher
around the other block; update both to call encodeURIComponent on the path
variables before building the URL (e.g., use encodeURIComponent(token) and
encodeURIComponent(hwid)) so reserved characters are safely escaped when
constructing `/sub/{token}/raw` and the hwid path, keeping the same orvalFetcher
call and argument names.
In `@tests/api/test_hwid.py`:
- Around line 45-46: The two intermediate calls using client.get(sub_url,
headers={"X-HWID": "device-2"}) and device-3 do not assert the response status;
update those calls to capture the Response objects (e.g., resp =
client.get(...)) and assert resp.status_code == 200 (or the expected HTTP
status) before proceeding to the later count/assertion; ensure you reference the
same variables (client, sub_url) and the X-HWID header when making the assertion
so failures surface as clear HTTP errors.
---
Nitpick comments:
In `@app/models/settings.py`:
- Around line 288-294: HWIDSettings currently only validates fields
individually, so invalid combos like min_limit > max_limit or fallback_limit
outside [min_limit, max_limit] can slip through; add a model-level validator
(e.g., a Pydantic `@root_validator` or model validator) on class HWIDSettings that
checks invariants: ensure min_limit <= max_limit, ensure fallback_limit is
between min_limit and max_limit (or equal to a defined sentinel behavior if
disabled), and raise a ValueError with a clear message if any invariant fails;
implement this in the HWIDSettings class so invalid configurations are rejected
at model creation.
In `@dashboard/src/components/layout/sidebar.tsx`:
- Around line 257-261: The object entry with title 'settings.hwid.title' in the
items array (the object containing url '/settings/hwid' and icon Fingerprint) is
missing a trailing comma; update that object literal to include a trailing comma
after its closing brace so it matches the other entries in the items array and
the project's consistent comma style.
In `@dashboard/src/features/templates/dialogs/user-template-modal.tsx`:
- Around line 179-184: The hwid_limit assignment in the submitData object is
doing a redundant null check because normalizedHwidLimit has already been
normalized to null or a number; simplify the expression in the submitData
construction (in user-template-modal.tsx) to directly test
Number.isFinite(normalizedHwidLimit) ? Math.floor(normalizedHwidLimit) : null
(or equivalent) so the code uses the existing normalizedHwidLimit variable and
removes the extra "== null" branch while preserving the current semantics.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 4e1b99be-1efe-47ac-b530-732773d13d71
📒 Files selected for processing (42)
app/db/crud/hwid.pyapp/db/crud/user.pyapp/db/migrations/versions/f02194c811d6_add_hwid_models_and_fields.pyapp/db/models.pyapp/models/settings.pyapp/models/subscription.pyapp/models/user.pyapp/models/user_template.pyapp/operation/hwid.pyapp/operation/subscription.pyapp/operation/user.pyapp/routers/__init__.pyapp/routers/dependencies/__init__.pyapp/routers/dependencies/_common.pyapp/routers/dependencies/subscription.pyapp/routers/hwid.pyapp/routers/subscription.pyapp/settings/__init__.pyapp/utils/wireguard.pydashboard/public/statics/locales/en.jsondashboard/public/statics/locales/fa.jsondashboard/public/statics/locales/ru.jsondashboard/public/statics/locales/zh.jsondashboard/src/app/router.tsxdashboard/src/components/layout/sidebar.tsxdashboard/src/features/templates/components/use-user-templates-list-columns.tsxdashboard/src/features/templates/components/user-template.tsxdashboard/src/features/templates/dialogs/user-template-modal.tsxdashboard/src/features/templates/forms/user-template-form.tsdashboard/src/features/users/components/action-buttons.tsxdashboard/src/features/users/components/users-table.tsxdashboard/src/features/users/dialogs/user-hwids-modal.tsxdashboard/src/features/users/dialogs/user-modal.tsxdashboard/src/features/users/dialogs/user-subscription-clients-modal.tsxdashboard/src/features/users/forms/user-form.tsdashboard/src/pages/_dashboard.bulk.create.tsxdashboard/src/pages/_dashboard.settings.hwid.tsxdashboard/src/pages/_dashboard.settings.tsxdashboard/src/pages/_dashboard.templates.user.tsxdashboard/src/service/api/index.tstests/api/conftest.pytests/api/test_hwid.py
Comment on lines
+44
to
+47
| db.add(new_hwid) | ||
| await db.commit() | ||
| await db.refresh(new_hwid) | ||
| return new_hwid |
There was a problem hiding this comment.
Make HWID registration idempotent under concurrent requests.
Concurrent calls can hit the unique constraint and raise an integrity error; this should be handled as a normal duplicate-registration case instead of surfacing a failure.
🛡️ Proposed fix
from sqlalchemy import delete, func, select
+from sqlalchemy.exc import IntegrityError
from sqlalchemy.ext.asyncio import AsyncSession
@@
db.add(new_hwid)
- await db.commit()
+ try:
+ await db.commit()
+ except IntegrityError:
+ await db.rollback()
+ existing = await get_user_hwid_by_value(db, user_id=user_id, hwid_str=hwid)
+ if existing is not None:
+ return existing
+ raise
await db.refresh(new_hwid)
return new_hwid📝 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.
Suggested change
| db.add(new_hwid) | |
| await db.commit() | |
| await db.refresh(new_hwid) | |
| return new_hwid | |
| from sqlalchemy import delete, func, select | |
| from sqlalchemy.exc import IntegrityError | |
| from sqlalchemy.ext.asyncio import AsyncSession | |
| db.add(new_hwid) | |
| try: | |
| await db.commit() | |
| except IntegrityError: | |
| await db.rollback() | |
| existing = await get_user_hwid_by_value(db, user_id=user_id, hwid_str=hwid) | |
| if existing is not None: | |
| return existing | |
| raise | |
| await db.refresh(new_hwid) | |
| return new_hwid |
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@app/db/crud/hwid.py` around lines 44 - 47, Wrap the db.add(new_hwid) / await
db.commit() / await db.refresh(new_hwid) block in a try/except that catches
sqlalchemy.exc.IntegrityError, call await db.rollback() inside the except, then
query the DB for the existing record using the same unique key(s) (e.g. by
hardware_id or whatever unique column the model uses) and return that existing
instance instead of raising; ensure IntegrityError is imported and that you only
suppress duplicates (re-raise other DB errors). Keep references to the current
variables/new_hwid and use the model's unique lookup (e.g. get_by_hardware_id or
a query on the model) to return the already-registered object.
Comment on lines
+969
to
+970
| if modify.hwid_limit is not None: | ||
| db_user.hwid_limit = modify.hwid_limit |
There was a problem hiding this comment.
Allow explicit clearing of hwid_limit during user modify.
This branch only runs for non-None values, so clients cannot reset an existing override to NULL (inherit/default behavior).
🐛 Proposed fix
- if modify.hwid_limit is not None:
- db_user.hwid_limit = modify.hwid_limit
+ if "hwid_limit" in modify.model_fields_set:
+ db_user.hwid_limit = modify.hwid_limit🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@app/db/crud/user.py` around lines 969 - 970, The modify logic for hwid_limit
only assigns when modify.hwid_limit is not None, preventing clients from
clearing an existing db_user.hwid_limit back to NULL; update the user-modify
handler (where modify.hwid_limit and db_user.hwid_limit are referenced) to
distinguish "no change" from "explicit clear" by adding an explicit flag or
using a sentinel (e.g., a separate boolean on the request or an Optional
wrapper) so that when the client signals clear you set db_user.hwid_limit =
None, otherwise leave it unchanged; locate the assignment involving
modify.hwid_limit and replace the conditional to apply the clear behavior when
the explicit-clear indicator is present.
Comment on lines
+280
to
+289
| # It's a new HWID, check limit | ||
| limit = user_hwid_limit if user_hwid_limit is not None else hwid_conf.fallback_limit | ||
| if limit == 0: | ||
| pass # unlimited | ||
| else: | ||
| current_count = await get_user_hwid_count(db, user_id) | ||
| if current_count >= limit: | ||
| await self.raise_error(message="Device limit reached", code=403) | ||
|
|
||
| await register_user_hwid(db, user_id, x_hwid, x_device_os, x_ver_os, x_device_model) |
There was a problem hiding this comment.
Enforce HWID limit atomically to prevent over-registration under concurrency.
Line 285–289 performs a read (get_user_hwid_count) then write (register_user_hwid) without a lock/atomic guard. Concurrent requests can bypass the cap and insert extra devices.
Suggested direction
- else:
- current_count = await get_user_hwid_count(db, user_id)
- if current_count >= limit:
- await self.raise_error(message="Device limit reached", code=403)
-
- await register_user_hwid(db, user_id, x_hwid, x_device_os, x_ver_os, x_device_model)
+ else:
+ # Move count+insert into one transactional CRUD function
+ # (e.g., SELECT ... FOR UPDATE on user/hwid rows, then insert).
+ created = await register_user_hwid_with_limit(
+ db=db,
+ user_id=user_id,
+ hwid=x_hwid,
+ limit=limit,
+ device_os=x_device_os,
+ ver_os=x_ver_os,
+ device_model=x_device_model,
+ )
+ if not created:
+ await self.raise_error(message="Device limit reached", code=403)
+ return
+
+ await register_user_hwid(db, user_id, x_hwid, x_device_os, x_ver_os, x_device_model)🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@app/operation/subscription.py` around lines 280 - 289, The HWID limit check
is non-atomic (uses get_user_hwid_count then register_user_hwid) and can be
bypassed by concurrent requests; fix by performing the check+insert inside a DB
transaction or by moving the enforcement into register_user_hwid so it does the
count-and-insert atomically (e.g., start a transaction, SELECT ... FOR UPDATE on
the user's row or use a DB constraint and attempt the insert, then raise the
same 403 error if the count >= limit). Update the code around
get_user_hwid_count and register_user_hwid to use this transactional/atomic
approach so concurrent requests cannot exceed the limit.
Comment on lines
+96
to
+100
| # Ensure everything is explicitly a Header parameter | ||
| if not isinstance(default, (HeaderParam, ParameterOverride)) and default is not Parameter.empty: | ||
| alias = field_info.alias if field_info.alias else field_name.replace("_", "-").title() | ||
| default = Header(default, alias=alias) | ||
|
|
There was a problem hiding this comment.
🧩 Analysis chain
🌐 Web query:
In FastAPI dependencies, if a function parameter is required (no default) and not wrapped in Header(...), is it treated as a query parameter by default?
💡 Result:
Yes, in FastAPI, if a function parameter (whether in a path operation function or a dependency function) is declared without a default value and is not explicitly wrapped in a parameter class like Header, Path, or Body, it is treated as a query parameter by default [1][2][3]. FastAPI determines the source of a parameter based on the following rules: 1. Path Parameters: If the parameter name is declared in the path string (e.g., /items/{item_id}), it is automatically treated as a path parameter [4]. 2. Special Types: Certain types, such as Request, Response, or BackgroundTasks, are automatically recognized and injected by FastAPI without needing any special declaration [2]. 3. Query Parameters: If a parameter is not a path parameter and is not one of the special types, it is treated as a query parameter by default [1][2]. Because you did not specify a default value, FastAPI will require this parameter to be present in the request's query string [1][3]. If the client fails to provide it, FastAPI will return a 422 Unprocessable Entity error.
Citations:
- 1: https://fastapi.tiangolo.com/tutorial/dependencies/
- 2: https://www.shanechang.com/p/understanding-fastapi-parameters/
- 3: https://medium.com/@likhith0715/fastapi-path-and-query-parameters-validation-metadata-query-models-and-pydantic-explained-89b8f05c82ce
- 4: https://fastapi.tiangolo.com/tutorial/path-params/
🏁 Script executed:
find . -type f -name "_common.py" | grep dependenciesRepository: PasarGuard/panel
Length of output: 98
🏁 Script executed:
cat -n app/routers/dependencies/_common.py | head -120 | tail -50Repository: PasarGuard/panel
Length of output: 2398
🏁 Script executed:
# Get more context around the function to understand the full logic
sed -n '80,110p' app/routers/dependencies/_common.pyRepository: PasarGuard/panel
Length of output: 1307
🏁 Script executed:
sed -n '40,72p' app/routers/dependencies/_common.pyRepository: PasarGuard/panel
Length of output: 1342
Required header fields are not bound as headers.
At lines 97–100, only non-Parameter.empty defaults are wrapped with Header(...). Required fields (Parameter.empty) will be interpreted as query parameters instead of headers, since FastAPI defaults to query parameters for undecorated required parameters.
Use Header(..., alias=alias) for required fields to ensure they are correctly sourced from request headers:
Proposed fix
- if not isinstance(default, (HeaderParam, ParameterOverride)) and default is not Parameter.empty:
- alias = field_info.alias if field_info.alias else field_name.replace("_", "-").title()
- default = Header(default, alias=alias)
+ alias = field_info.alias if field_info.alias else field_name.replace("_", "-").title()
+ if not isinstance(default, HeaderParam):
+ if default is Parameter.empty:
+ default = Header(..., alias=alias)
+ else:
+ default = Header(default, alias=alias)🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@app/routers/dependencies/_common.py` around lines 96 - 100, The code
currently only wraps non-required defaults in Header(...) so required parameters
(default == Parameter.empty) remain unbound and become query params; update the
logic around default/field_info/field_name in the block that checks
isinstance(default, (HeaderParam, ParameterOverride)) to also wrap required
parameters by calling Header(..., alias=alias) when default is Parameter.empty
(use the same alias calculation using field_info.alias or
field_name.replace("_", "-").title()), so both required and optional header
fields are explicitly bound as Header with the correct alias.
Comment on lines
+139
to
+141
| <Button type="button" variant="ghost" size="icon" className="h-8 w-8 text-destructive hover:text-destructive" onClick={() => setHwidToDelete(item.hwid)}> | ||
| <Trash2 className="h-4 w-4" /> | ||
| </Button> |
There was a problem hiding this comment.
Add an accessible name to the icon-only remove action.
Line 139 renders a destructive icon-only button without an accessible label, so screen-reader users can’t reliably identify the action.
♿ Proposed fix
- <Button type="button" variant="ghost" size="icon" className="h-8 w-8 text-destructive hover:text-destructive" onClick={() => setHwidToDelete(item.hwid)}>
+ <Button
+ type="button"
+ variant="ghost"
+ size="icon"
+ className="h-8 w-8 text-destructive hover:text-destructive"
+ aria-label={t('remove', { defaultValue: 'Remove' })}
+ title={t('remove', { defaultValue: 'Remove' })}
+ onClick={() => setHwidToDelete(item.hwid)}
+ >
<Trash2 className="h-4 w-4" />
</Button>📝 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.
Suggested change
| <Button type="button" variant="ghost" size="icon" className="h-8 w-8 text-destructive hover:text-destructive" onClick={() => setHwidToDelete(item.hwid)}> | |
| <Trash2 className="h-4 w-4" /> | |
| </Button> | |
| <Button | |
| type="button" | |
| variant="ghost" | |
| size="icon" | |
| className="h-8 w-8 text-destructive hover:text-destructive" | |
| aria-label={t('remove', { defaultValue: 'Remove' })} | |
| title={t('remove', { defaultValue: 'Remove' })} | |
| onClick={() => setHwidToDelete(item.hwid)} | |
| > | |
| <Trash2 className="h-4 w-4" /> | |
| </Button> |
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@dashboard/src/features/users/dialogs/user-hwids-modal.tsx` around lines 139 -
141, The icon-only remove Button in user-hwids-modal.tsx lacks an accessible
name; update the Button (the element rendering <Trash2 /> and calling
setHwidToDelete(item.hwid)) to provide one—either add an explicit aria-label
(e.g., aria-label={`Remove HWID ${item.hwid}`} or aria-label="Remove hardware
ID") or include visually hidden text (a <span className="sr-only">Remove
hardware ID</span>) inside the Button so screen readers can identify the action.
| username: z.string().min(3, 'validation.minLength').max(128, 'validation.maxLength'), | ||
| group_ids: z.array(z.number()).min(1, { message: 'validation.required' }), | ||
| data_limit: z.number().min(0), | ||
| hwid_limit: z.number().min(0).nullable().optional(), |
There was a problem hiding this comment.
Enforce integer validation for hwid_limit.
Line 54 accepts fractional values for a device-count field. This can lead to unintended policy outcomes downstream during submission normalization.
🛡️ Proposed fix
- hwid_limit: z.number().min(0).nullable().optional(),
+ hwid_limit: z.number().int().min(0).nullable().optional(),📝 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.
Suggested change
| hwid_limit: z.number().min(0).nullable().optional(), | |
| hwid_limit: z.number().int().min(0).nullable().optional(), |
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@dashboard/src/features/users/forms/user-form.ts` at line 54, The hwid_limit
schema currently allows fractional numbers; update the validation for hwid_limit
in user-form.ts to require non-negative integers by adding integer enforcement
(use the z.number().int().min(0) form) while preserving nullable() and
optional() so the field can still be null/omitted; update the hwid_limit
definition to use the integer-constrained validator to prevent fractional device
counts.
Comment on lines
+79
to
+82
| const handleCancel = () => { | ||
| form.reset(formValues) | ||
| toast.success(t('settings.hwid.cancelSuccess', { defaultValue: 'Changes cancelled and original HWID settings restored' })) | ||
| } |
There was a problem hiding this comment.
Unusual UX: success toast on cancel action.
Showing a success toast when canceling changes is non-standard. Users typically expect success feedback only for completed actions (like saving). Consider either removing the toast entirely or using a neutral message instead of toast.success.
Suggested alternatives
Option 1: Remove the toast entirely
const handleCancel = () => {
form.reset(formValues)
- toast.success(t('settings.hwid.cancelSuccess', { defaultValue: 'Changes cancelled and original HWID settings restored' }))
}Option 2: Use neutral info toast
const handleCancel = () => {
form.reset(formValues)
- toast.success(t('settings.hwid.cancelSuccess', { defaultValue: 'Changes cancelled and original HWID settings restored' }))
+ toast.info(t('settings.hwid.cancelInfo', { defaultValue: 'Changes discarded' }))
}📝 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.
Suggested change
| const handleCancel = () => { | |
| form.reset(formValues) | |
| toast.success(t('settings.hwid.cancelSuccess', { defaultValue: 'Changes cancelled and original HWID settings restored' })) | |
| } | |
| const handleCancel = () => { | |
| form.reset(formValues) | |
| } |
Suggested change
| const handleCancel = () => { | |
| form.reset(formValues) | |
| toast.success(t('settings.hwid.cancelSuccess', { defaultValue: 'Changes cancelled and original HWID settings restored' })) | |
| } | |
| const handleCancel = () => { | |
| form.reset(formValues) | |
| toast.info(t('settings.hwid.cancelInfo', { defaultValue: 'Changes discarded' })) | |
| } |
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@dashboard/src/pages/_dashboard.settings.hwid.tsx` around lines 79 - 82, The
cancel handler currently shows a success toast which is non-standard; update the
handleCancel function so it no longer calls toast.success(...) — either remove
the toast call entirely or replace it with a neutral/info toast (e.g.,
toast.info) and/or a less-assertive message key
(t('settings.hwid.cancelSuccess', ...) can be replaced or removed); keep the
existing form.reset(formValues) behavior and adjust the toast usage accordingly
in handleCancel to avoid showing a success state for a cancel action.
Comment on lines
+11577
to
+11579
| export const userSubscriptionRaw = (token: string, signal?: AbortSignal) => { | ||
| return orvalFetcher<unknown>({ url: `/sub/${token}/raw`, method: 'GET', signal }) | ||
| } |
There was a problem hiding this comment.
URL path params should be URI-encoded before interpolation
Line 11578 (token) and Line 12391 (hwid) are inserted into URL paths without encoding. If they contain reserved characters (/, #, %, ?), requests can hit the wrong route or fail.
🔧 Proposed fix
export const userSubscriptionRaw = (token: string, signal?: AbortSignal) => {
- return orvalFetcher<unknown>({ url: `/sub/${token}/raw`, method: 'GET', signal })
+ return orvalFetcher<unknown>({ url: `/sub/${encodeURIComponent(token)}/raw`, method: 'GET', signal })
}
export const deleteUserHwid = (userId: number, hwid: string) => {
- return orvalFetcher<unknown>({ url: `/api/user/${userId}/hwids/${hwid}`, method: 'DELETE' })
+ return orvalFetcher<unknown>({ url: `/api/user/${userId}/hwids/${encodeURIComponent(hwid)}`, method: 'DELETE' })
}Also applies to: 12390-12392
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@dashboard/src/service/api/index.ts` around lines 11577 - 11579, The URL path
parameters are interpolated without encoding in userSubscriptionRaw (token) and
the similar hwid-based fetcher around the other block; update both to call
encodeURIComponent on the path variables before building the URL (e.g., use
encodeURIComponent(token) and encodeURIComponent(hwid)) so reserved characters
are safely escaped when constructing `/sub/{token}/raw` and the hwid path,
keeping the same orvalFetcher call and argument names.
Comment on lines
+45
to
+46
| client.get(sub_url, headers={"X-HWID": "device-2"}) | ||
| client.get(sub_url, headers={"X-HWID": "device-3"}) |
There was a problem hiding this comment.
Validate response status for intermediate HWID registrations.
The subscription requests at lines 45-46 don't check the response status. If either registration fails, the test will fail at line 49 with a confusing count mismatch instead of a clear HTTP error.
✅ Proposed fix to add status code assertions
- client.get(sub_url, headers={"X-HWID": "device-2"})
- client.get(sub_url, headers={"X-HWID": "device-3"})
+ response = client.get(sub_url, headers={"X-HWID": "device-2"})
+ assert response.status_code == status.HTTP_200_OK
+ response = client.get(sub_url, headers={"X-HWID": "device-3"})
+ assert response.status_code == status.HTTP_200_OK📝 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.
Suggested change
| client.get(sub_url, headers={"X-HWID": "device-2"}) | |
| client.get(sub_url, headers={"X-HWID": "device-3"}) | |
| response = client.get(sub_url, headers={"X-HWID": "device-2"}) | |
| assert response.status_code == status.HTTP_200_OK | |
| response = client.get(sub_url, headers={"X-HWID": "device-3"}) | |
| assert response.status_code == status.HTTP_200_OK |
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@tests/api/test_hwid.py` around lines 45 - 46, The two intermediate calls
using client.get(sub_url, headers={"X-HWID": "device-2"}) and device-3 do not
assert the response status; update those calls to capture the Response objects
(e.g., resp = client.get(...)) and assert resp.status_code == 200 (or the
expected HTTP status) before proceeding to the later count/assertion; ensure you
reference the same variables (client, sub_url) and the X-HWID header when making
the assertion so failures surface as clear HTTP errors.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
2 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary by CodeRabbit
New Features
Chores