Files

wangziqi 1289aea7ce docs(ocr): 全库 OCR 审查与修复记录

由 OCR（open-codereview.ai，deepseek-v4-flash）审查产出：
- 审查汇总.md / high-severity-comments.md / all-comments.jsonl / 修复记录.md 等

Reviewed-by: OCR (open-codereview.ai)

2026-08-09 21:29:54 +08:00

64 KiB

Raw Blame History

High 级问题清单（OCR 审查）

共 88 条，按文件分组：

apps/server/src/classes/classes.controller.ts（3）

[security] L147-149 All mutation endpoints in this controller (update, archive, restore, remove, purge, batchImportStudents, addStudents, removeStudent, addTeacher, removeTeacher, removeTeacherAssignment) rely only on the global @RequirePermission('class:edit') and never call await this.assertReadAccess(req, +id). The corresponding service methods (ClassesService.update/archive/purge/addStudents/addTeacher...) only perform existence checks, not membership checks. Since the read endpoints (findOne, getSchedule, getStudents, ...) do enforce assertReadAccess, a user who holds class:edit but is only assigned to some classes (e.g., a subject teacher) can modify, archive, purge, or import/remove students in ANY class — a broken-access-control / IDOR vulnerability. Add await this.assertReadAccess(req, +id) to every mutation endpoint, consistent with the read path.
[security] L167-169 Destructive endpoint: purge permanently deletes a class, but unlike the read endpoints it never calls assertReadAccess(req, +id), so any caller with the class:purge/class:edit permission can permanently delete a class they were never assigned to. Add await this.assertReadAccess(req, +id) before calling the service.
[security] L127-129 Bulk import endpoint imports students (by DingTalk userId) into a class without any class-level access check — same missing assertReadAccess(req, +id) as the other mutation endpoints. A user with only class:edit can inject students into any class they are not assigned to.

deploy.sh（3）

[bug] L34-39 Remote commands run without set -e (only the local shell enables set -euo pipefail). The ssh exit code is determined solely by the last remote command (pm2 status), so if npm ci or npm run migration:run -w @gongxue/server fails, the script still reloads PM2 and prints "部署完成！" — a failed migration is silently swallowed and the deploy is reported as successful. Add set -euo pipefail at the start of the remote command string (right after cd ${REMOTE_DIR}) or check each step's exit code explicitly.
[security] L17-24 --delete will remove any server-only files that are not excluded. The repo contains no committed .env (only .env.example), so the production .env created manually on the server would be deleted on every deploy, and a local dev .env (if present) would overwrite the production one — leaking/erasing secrets. Add --exclude='.env' / --exclude='.env.*' and exclude any other server-only runtime data (e.g. uploads/) that must survive deploys.
[bug] L30-33 npm ci runs only when node_modules does not exist on the server. On subsequent deploys, changes to package.json/package-lock.json (new or removed dependencies) are never installed because node_modules is excluded from rsync and already present remotely — production can run with stale dependencies while the local build (used to produce the deployed bundle) was built with new ones. Always run npm ci --omit=dev on the server, or trigger it by comparing the remote and local package-lock.json checksums.

apps/server/src/archive/archive.controller.ts（2）

[security] L203-204 FileInterceptor is used without any limits and defaults to multer memory storage, so the entire uploaded file is buffered in RAM with no size cap (multer's default file size limit is unlimited). A malicious/large upload can exhaust server memory (DoS), and arbitrary file types are accepted. Configure limits: { fileSize: ... } and validate the content type / file signature before persisting.
```
@UseInterceptors(FileInterceptor('file', { limits: { fileSize: 10 * 1024 * 1024 } }))
async uploadAttachment(
```
[security] L226-227 Stored XSS risk: mimeType is stored verbatim from the client-supplied upload header (see addAttachment in the service) and later served with Content-Disposition: inline from the same origin. An attacker can upload a .html/.svg file claiming text/html and it will render/execute in the app origin when downloaded. Use attachment disposition (forces download instead of inline rendering) and/or whitelist safe content types when serving.
```
  res.setHeader('Content-Disposition', `attachment; filename="${encodeURIComponent(fileName)}"`);
```

apps/server/src/dashboard/dashboard.controller.ts（2）

[security] L46-49 Authorization/data-scoping inconsistency: unlike getStats/getClassAttendanceRanking, these endpoints never resolve the requesting user's accessible class IDs. getGanttData, getExpenseStats and getRoomExpenseRanking return global (all rooms/expenses) data to any authenticated user holding the dashboard:view permission, even non-admin teachers — a horizontal privilege escalation / sensitive data exposure (esp. expense figures). Either pass req through getAccessibleClassIds(req) (and have the service scope the queries) or restrict these routes to admins/super-admins explicitly.
[security] L66-69 Same authorization gap as the gantt/expense-stats/room-ranking endpoints: getClassroomOccupancy and getClassroomUtilization ignore the requesting user and expose global classroom schedule/rental data to any user with dashboard:view, including non-admin teachers. These should be scoped by the caller's accessible class IDs or explicitly restricted to admin-only roles.

apps/server/src/dashboard/dto/dashboard-query.dto.ts（2）

[bug] L5-7 Contradictory validation rules make this property impossible to validate successfully. @Matches(/^\d{4}-\d{2}-\d{2}$/) only accepts a bare YYYY-MM-DD date, but @IsISO8601({ strict: true }) (validator.js strict mode) requires a full ISO 8601 timestamp with the T separator, time and timezone (e.g. 2024-01-01T00:00:00.000Z). Because class-validator requires all constraints on a property to pass, no value can ever satisfy both — every request using this DTO will be rejected with a 400. Pick one format: e.g. drop strict: true so @IsISO8601() accepts plain dates, or align the regex to the strict ISO format.
```
@Matches(/^\d{4}-\d{2}-\d{2}$/)
@IsISO8601()
periodStart?: string;
```
[bug] L10-12 Same contradiction as periodStart: @Matches(/^\d{4}-\d{2}-\d{2}$/) allows only a date-only string while @IsISO8601({ strict: true }) requires a full timestamp with time and timezone, so the two constraints can never pass together. Align the formats (e.g. remove strict: true if only YYYY-MM-DD is intended).
```
@Matches(/^\d{4}-\d{2}-\d{2}$/)
@IsISO8601()
periodEnd?: string;
```

apps/server/src/entities/ding-leave-raw.entity.ts（2）

[bug] L56-57 Type mismatch: the column is nullable: true and the relation uses onDelete: 'SET NULL', so matchedStudentId will legitimately be null for unmatched records (confirmed by usage such as raw.matchedStudentId == null in attendance-lesson.service.ts). Declaring it as number lets TypeScript assume it is never null, defeating null-safety checks and risking NPEs/undefined behavior when accessing it. Declare it as number | null.
```
@Column({ name: 'matched_student_id', type: 'integer', nullable: true })
matchedStudentId: number | null;
```
[bug] L59-61 Since the FK column can be null (onDelete: 'SET NULL', nullable: true), the relation property should be typed as Student | null. With matchedStudent: Student, TS assumes the object is always present and won't flag accesses like record.matchedStudent.name without a null check, which can cause runtime errors for unmatched records.
```
@ManyToOne(() => Student, { onDelete: 'SET NULL', nullable: true })
@JoinColumn({ name: 'matched_student_id' })
matchedStudent: Student | null;
```

apps/server/src/exams/exams.controller.ts（2）

[bug] L40-42 exam:edit is listed in DEPRECATED_PERMISSION_CODES (rbac-presets.ts) and is actively removed from every role and deleted from the permission table by rbac-seed.service.ts. As a result, for any non-superadmin user req.user.permissions.includes('exam:edit') can never be true, so the "manage all" branch is effectively dead code and users can never obtain global scope through RBAC (e.g., a user holding the active exam:purge permission would still be restricted to their own scope when purging). Use an active permission code or rework the scope model.
```
private canManageAll(req: AuthenticatedRequest) {
  return req.user.isSuperAdmin || req.user.permissions.includes('exam:view');
}
```
[security] L85-87 Mutating endpoints (create, archive, restore, batch-archive, batch-restore, updateScore) are protected only by @RequirePermission('exam:view'), a view-level permission. Any user granted read access can create exams, archive/restore them, and overwrite scores — inconsistent with every other module in this codebase where create/edit/delete operations require dedicated :create/:edit/:delete codes. Since exam:create/exam:edit were deprecated, at minimum introduce a distinct edit permission for these mutations (or enforce an explicit scope/ownership check at the guard level) to preserve least privilege.

apps/server/src/expenses/expenses.controller.ts（2）

[bug] L354-357 Missing null check for the uploaded file: if the request contains no file part (e.g., wrong field name or file not attached), file is undefined and file.buffer throws a TypeError, producing an unhandled 500. Also, workbook.xlsx.load throws a generic error for invalid/non-xlsx content with no user-friendly message. Add a guard and wrap parsing in try/catch, throwing BadRequestException (imported from '@nestjs/common').

@UseInterceptors(FileInterceptor('file'))
async importUtilityExpenses(@UploadedFile() file: Express.Multer.File, @Request() req: AuthenticatedRequest) {
  if (!file) throw new BadRequestException('请上传 Excel 文件');
  const workbook = new ExcelJS.Workbook();
  try {
    await workbook.xlsx.load(bufferToArrayBuffer(file.buffer));
  } catch {
    throw new BadRequestException('文件格式无效，请上传 .xlsx 模板文件');
  }

[bug] L419-422 Same issue as the utility import endpoint: file can be undefined when the request has no file part, and file.buffer will throw an unhandled TypeError (500). Invalid Excel content also surfaces as a generic error. Add a null guard and a try/catch that throws a user-friendly BadRequestException.

@UseInterceptors(FileInterceptor('file'))
async importPersonalExpenses(@UploadedFile() file: Express.Multer.File, @Request() req: AuthenticatedRequest) {
  if (!file) throw new BadRequestException('请上传 Excel 文件');
  const workbook = new ExcelJS.Workbook();
  try {
    await workbook.xlsx.load(bufferToArrayBuffer(file.buffer));
  } catch {
    throw new BadRequestException('文件格式无效，请上传 .xlsx 模板文件');
  }

apps/server/src/expenses/expenses.service.ts（2）

[bug] L138-144 status is returned in the result mapping (status: String(row.status)) but e.status is never added to roomExpenseSelects. getRawMany only returns the selected columns, so row.status is always undefined and every agent-search result will report status: "undefined" instead of active. Add ['e.status', 'status'] to the selects (and likewise for the personal-expense query below).
```
  const roomExpenseSelects = [
    ['e.expenseType', 'expenseType'],
    ['e.amount', 'amount'],
    ['e.periodStart', 'periodStart'],
    ['e.periodEnd', 'periodEnd'],
    ['room.roomNumber', 'roomNumber'],
    ['e.status', 'status'],
  ] as const;
```
[bug] L167-173 Same issue as the room query: personalExpenseSelects does not include e.status, yet the mapping below calls String(row.status). Since getRawMany only returns selected columns, row.status is undefined and every result reports status: "undefined". Add ['e.status', 'status'] to the selects.
```
  const personalExpenseSelects = [
    ['e.expenseType', 'expenseType'],
    ['e.amount', 'amount'],
    ['e.expenseDate', 'expenseDate'],
    ['student.name', 'studentName'],
    ['student.studentNo', 'studentNo'],
    ['e.status', 'status'],
  ] as const;
```

migrate.sh（2）

[security] L19-19 密码以 -p"${MYSQL_PASS}" 形式拼在命令行参数中，会出现在 ps aux 进程列表里，同机其他用户可直接看到明文密码。建议改用 docker compose exec -T -e MYSQL_PWD="${MYSQL_PASS}" mysql mysql -u root ... 方式传入，避免密码暴露在进程列表中。

gunzip -c "$DUMP" | docker compose exec -T -e MYSQL_PWD="${MYSQL_PASS}" mysql mysql -u root dorm_billing

- **[security] L37-37** 脚本最后把含明文密码的完整命令 `-p${MYSQL_PASS}` echo 到终端/日志，直接泄露凭据（也会进入 CI/终端历史）。建议只输出不含密码的验证命令，例如提示使用 MYSQL_PWD 或提示输入密码。

echo " docker compose exec -e MYSQL_PWD= mysql mysql -u root gongxue -e 'SELECT COUNT(*) FROM students'"


## .gitea/workflows/deploy.yml（2）
- **[security] L53-56** `--delete` will remove any files on the server that are not present in the source tree. Runtime config such as `.env` / `.env.*` (typically gitignored and thus absent from the repo) will be silently deleted on every deploy, breaking DB credentials and other environment settings. Add `--exclude='.env'` (and `--exclude='.env.*'`) or keep server-side config outside REMOTE_DIR.
- **[bug] L69-72** Dependencies are only installed when `node_modules` does not exist. On subsequent deploys, changes to `package.json` / `package-lock.json` will never be installed, so the server keeps running stale dependency versions after the code has been updated. Run `npm ci --omit=dev` unconditionally (or compare a lockfile hash / mtime) instead of gating on directory existence.

## apps/admin/src/api/schemas/attendance.ts（1）
- **[bug] L53-53** Schema mismatch with the actual API response. The server's `/attendance-records/alerts` endpoint (`getAlerts` in attendance-report.service.ts) returns items shaped `{ studentId, studentName, className, type, count, lastDate }` (and the admin `AlertItem` type also declares `studentNo`), but this schema requires `id` and `message`, which the API never returns. Since these fields are required in Zod, `validateResponse` will throw for every alert item and the alerts query will always fail at runtime. Align the schema with the real response shape, e.g. include `studentId/studentName/studentNo/className/type/count/lastDate`.

.object({ studentId: z.number(), studentName: z.string(), studentNo: z.string().nullable().optional(), className: z.string(), type: z.string(), count: z.number(), lastDate: z.string(), })


## apps/admin/src/components/AiChat/useSubmissionState.ts（1）
- **[bug] L77-82** This block mutates refs (commandsRef / idRef) during the render phase to detect a surfaceId change. This is a render-phase side effect: it violates React's rules and is unsafe under StrictMode double-rendering/concurrent rendering (an aborted render can leave the refs mutated while the committed output still belongs to the old surface). It also leaves the returned `commands` state out of sync: the refs are cleared but the `commands` state array is not reset, so consumers keep receiving the previous surface's commands until the next `pushCommands` call. Move the reset into an effect (and clear the state in sync), e.g.:

```ts
const surfaceKey = surfaceId;
useEffect(() => {
if (idRef.current !== surfaceKey) {
  commandsRef.current = [];
  idRef.current = surfaceKey;
  setCommands([]);
}
}, [surfaceKey]);

Alternatively, remount the consumer component per surface with key={surfaceId}.

apps/admin/src/components/AiChat/DynamicChart.tsx（1）

[bug] L204-208 The empty/undefined data guard runs after useMemo has already invoked buildOption(chart). Every builder (buildNameValueRows, buildCategoryOption, etc.) immediately calls chart.rows.map(...) and chart.columns[0]?.key, so if rows/columns is missing at runtime this throws before the "暂无数据" placeholder is reached. The chart payload comes from AI message metadata that is only validated as typeof payload === 'object' (see deriveCharts in uiArtifacts.ts), so a missing rows array is entirely possible and the !chart.rows guard itself acknowledges it — the guard is just ineffective because of ordering. Fix: make the option build null-safe instead of moving the guard above the hook (that would violate the Rules of Hooks), e.g. only build the option when rows exist, and guard columns in the builders.
```
const option = useMemo<EChartsOption>(
  () => (chart && chart.rows?.length ? buildOption(chart) : {}),
  [chart],
);
const [instance, setInstance] = useState<EChartsType | null>(null);
if (!chart) return null;
// 空数据集：渲染明确占位，而不是一张空白图
if (!chart.rows || chart.rows.length === 0) {
```

apps/admin/src/components/JinshujuMatchModal.tsx（1）

[bug] L145-149 When the apply API returns success: false, the if (res.success) block is skipped, nothing is thrown, and the catch never runs — so the modal stays permanently stuck on the 'applying' loading screen with no error message and no way to return to the match step (footer is null). This is inconsistent with handlePreview and handleConnectionNext error handling. Also, res.log may be undefined, so res.log.message can throw a TypeError that leaves the user without feedback. Handle the failure branch explicitly.
```
    if (res.success) {
      message.success(res.log?.message || `处理 ${res.log?.recordsCount ?? 0} 条记录`);
      onApplied();
      reset();
    } else {
      throw new Error(res.log?.message || '同步失败');
    }
```

apps/admin/src/components/EditableCell/index.tsx（1）

[bug] L265-269 Pressing Enter while using editors whose value commits through their own internal Enter handling (date/date-range/select/multi-select/tags) will also hit this keydown handler and call save() with the draft captured in the current render. Those controls only update draft via their own async onChange (e.g., rc-picker commits a typed date, rc-select selects a highlighted option, tags mode adds a tag), so draft here is still the pre-interaction stale value. Depending on event ordering, event.preventDefault() may also suppress the control's commit — the user's typed date / selected option / typed tag is either silently discarded or the previous value gets saved over the new one (and two concurrent onSave calls can race). Restrict the Enter shortcut to editors whose draft is already in sync with the input (text/textarea/number/money).
```
  if (
    event.key === 'Enter' &&
    (editor === 'text' || editor === 'textarea' || editor === 'number' || editor === 'money')
  ) {
    event.preventDefault();
    await save();
    return;
  }
```

apps/admin/src/components/StudentProfileContent/AttachmentsTab.tsx（1）

[security] L83-85 Opening user-uploaded attachment bytes via URL.createObjectURL + window.open(url, '_blank') is a stored-XSS vector: a blob URL inherits the creator page's origin, so if the attachment is an HTML/SVG file containing scripts, those scripts run in the admin's origin/session. Prefer forcing a download instead — e.g. build an <a> with a download attribute and click it, set the blob type to application/octet-stream, or serve the file through a dedicated endpoint returning Content-Disposition: attachment + X-Content-Type-Options: nosniff. Also note window.open may return null when blocked by popup blockers, and revoking the URL after a fixed 60s can break a preview tab that stays open longer.

apps/admin/src/pages/Attendance/AttendanceAdmin.helpers.tsx（1）

[bug] L192-192 'late' records are excluded from the attendance rate calculation and from pickPrimaryStatus priority. Since attendance-workspace.ts treats status === 'late' as checked-in, a student with only 'late' records will be shown with primaryStatus 'absent' and rate 0%, misrepresenting them as fully absent. Count late as checked-in (consistent with the workspace logic) and add 'late' to the priority list in pickPrimaryStatus.

const checked = item.records.filter( (record) => record.status === 'present' || record.status === 'late', ).length;


## apps/admin/src/pages/Attendance/index.tsx（1）
- **[bug] L9-12** `useMemo(readCurrentRoles, [])` reads the store only once at first render via `getState()` and never subscribes to updates. `userStore.user` is initialized as `null` and populated only after login/persist rehydration, so if this page mounts before the user data is available (or after role changes/logout), `roles` stays `[]` and `experience` is permanently stale — e.g. an academic admin (`教务管理员`) without an explicit `attendance:edit` permission would be wrongly routed to the teacher workspace. Use a zustand selector instead so the component subscribes and recomputes when roles change:

```tsx
const roles = useUserStore((state) =>
Array.isArray(state.user?.roles) ? (state.user!.roles as string[]) : [],
);

const roles = useUserStore((state) =>
Array.isArray(state.user?.roles) ? (state.user!.roles as string[]) : [],
);

apps/admin/src/pages/Expenses/ExpenseTablePanel.tsx（1）

[performance] L110-119 EditableExpenseCell is declared inside the ExpenseTablePanel render body, so every parent render creates a brand-new component type. React then treats all editable cells as different component types and unmounts/remounts them on each render (e.g. when loading, selectedKeys, data, or filters change), which resets EditableCell's internal editing state (losing focus / unsaved input) and wastes reconciliation work across the whole table. Hoist this component to module scope and pass permission/disabled as props, or render cells via a plain function instead of JSX components.

apps/admin/src/pages/Roles/index.tsx（1）

[bug] L386-389 Functional bug: each Checkbox.Group is an independent component whose onChange only reports the checked values within that group's own options. Replacing the whole selectedPermIds with vals therefore wipes out the permission selections of all other groups the moment the user toggles any checkbox in one group (the header “全选” checkboxes are fine because they merge via setSelectedPermIds(prev => ...), but individual permission checkboxes are not). Fix: merge group-local changes into the existing selection, e.g. remove the current group's ids first and then append vals.
```
                <Checkbox.Group
                  value={selectedPermIds}
                  onChange={(vals) => {
                    const groupIds = new Set(
                      allPerms.find((g) => g.group === group.group)?.permissions.map((p) => p.id) ?? [],
                    );
                    setSelectedPermIds((prev) => [
                      ...prev.filter((id) => !groupIds.has(id)),
                      ...(vals as number[]),
                    ]);
                  }}
                >
```

apps/admin/src/pages/Schedules/ScheduleModals.tsx（1）

[bug] L80-82 The useDirtyGuard hook returns a freshly-created object literal { confirmClose, snapshot, isDirty } on every render (see useDirtyGuard.ts). Because this entire object is in the effect's dependency array, this effect re-runs after every render of ScheduleModal while open && mode !== 'detail', calling scheduleGuard.snapshot() each time. That resets the pristine baseline to the current form values, so the dirty-guard's isDirty check nearly always returns false and unsaved changes are silently discarded when closing the modal — the exact protection the guard is meant to provide. Since snapshot is memoized with useCallback and is stable, depend on it instead of the wrapper object.
```
useEffect(() => {
  if (open && mode !== 'detail') scheduleGuard.snapshot();
}, [open, mode, editingSchedule, scheduleGuard.snapshot]);
```

apps/admin/src/pages/Schedules/index.tsx（1）

[bug] L343-345 Bug: this guard blocks creating a schedule from the month view. In month view, handleDateClick opens the detail modal with selectedCell = null (only selectedDate is set), and the detail modal's "新增排课" button calls onStartCreate, which switches to modalMode === 'create' while keeping selectedCell null. handleSubmit then returns early and submission silently fails (no message). The guard should also allow creation when selectedDate is present.
```
const handleSubmit = async () => {
  if (modalMode === 'create' && !selectedCell && !selectedDate) return;
  if (modalMode === 'edit' && !editingSchedule) return;
```

apps/server/src/ai-chat/ai-a2ui-submissions.service.ts（1）

[bug] L27-29 TOCTOU race breaks the idempotency guarantee: findSubmission followed by save is not atomic. Under concurrent duplicate submissions (double-click / parallel retries with the same artifactId+clientRequestId), both requests can pass the existence check, and the second save will hit the unique index uk_ai_a2ui_submissions_artifact_client, throwing an unhandled duplicate-key error (QueryFailedError) and returning a 500 to the caller instead of the existing submission. Recommend an atomic upsert (e.g. insert with ON DUPLICATE KEY/ON CONFLICT DO NOTHING depending on DB) or catching the unique-violation error and re-reading the existing row in the conflict path.

apps/server/src/ai-chat/ai-excel-reader.service.ts（1）

[bug] L84-84 The truncated flag semantics are inverted. slice.length < limit means the end of the sheet was reached (i.e., nothing was cut off), while the truly truncated case is when exactly limit rows were returned but the sheet still has more rows (from + limit < sheet.rows.length). As written, the last page reports truncated: false and an earlier partial page can misreport, which can mislead the AI into stopping early or reading past the end.
```
    truncated: from + limit < sheet.rows.length,
```

apps/server/src/ai-chat/ai-review.shared.ts（1）

[bug] L0-0 The AiReviewSectionResult union declares the same member twice ({ created: ... } in both branches) — the second branch is a copy-paste typo and should be { completed: ... }. As written, the union collapses to a single shape, so the completed variant is no longer part of the type: importTransfers/importCheckins/parseStoredSectionResult return { completed, ... } objects that are not assignable to this type, and callers in mergeStepResult/sectionResultMessage are forced to use unsafe as casts. Fix the second member.

export type AiReviewSectionResult = | { created: number; skipped: number; issues: string[] } | { completed: number; skipped: number; issues: string[] };


## apps/server/src/ai-chat/ai-review.import-relations.ts（1）
- **[bug] L192-202** 同一学生在同一批次中，如果一行只填手机号、另一行只填学号（或一行同时填了学号+手机号），去重 key 和数据库查询都只按单一标识（phone 优先）进行，第二条记录无法命中第一条已创建的学生，会重复创建 Student（重复学号/手机号），破坏数据唯一性。建议去重 key 同时覆盖两个标识，且查询时 phone 查不到再按 studentNo 查。

const dedupeKey = `${phone ? `phone:${phone}` : ''}|${studentNo ? `no:${studentNo}` : ''}`;
if (seen.has(dedupeKey)) {
  skipped += 1;
  issues.push(`学生「${name}」与同一批次中的其他行手机号/学号重复`);
  continue;
}
seen.add(dedupeKey);

let student = phone
  ? await studentRepo.findOne({ where: { phone } })
  : null;
if (!student && studentNo) {
  student = await studentRepo.findOne({ where: { studentNo } });
}


## apps/server/src/ai-chat/ai-review.import-basic.ts（1）
- **[bug] L174-176** Timezone-dependent off-by-one bug: the date is parsed as midnight `+08:00`, but `setDate`/`getFullYear`/`getMonth`/`getDate` use the server's *local* timezone. On a UTC server (or any zone west of +08:00), `nextDay('2026-08-09')` parses to 2026-08-08T16:00Z, so the local date is the 8th; adding one day then formatting yields `2026-08-09` — the same day, not the next day. Since this feeds `billingStartDate` for transfers, billing dates can be off by one day. Also, an invalid `date` string silently produces `NaN-NaN-NaN`. Use UTC-based getters (or a date library) and validate the input.

export function nextDay(date: string): string { const parsed = new Date(${date}T00:00:00Z); if (Number.isNaN(parsed.getTime())) return date; parsed.setUTCDate(parsed.getUTCDate() + 1); const year = parsed.getUTCFullYear(); const month = String(parsed.getUTCMonth() + 1).padStart(2, '0'); const day = String(parsed.getUTCDate()).padStart(2, '0'); return ${year}-${month}-${day}; }


## apps/server/src/app.module.ts（1）
- **[bug] L168-168** synchronize defaults to `true` whenever DB_SYNCHRONIZE is not set, including production, and the loose `!== 'false'` comparison treats any value other than the literal string 'false' (e.g. '0', 'no', '', 'TRUE') as enabling auto-sync. Combined with the large entity list and the migrations array defined just above, TypeORM auto-sync can silently alter/drop columns and conflict with the migration history, risking real data loss. Recommend defaulting to `false` in production and parsing strictly, e.g.: synchronize: config.get('DB_SYNCHRONIZE', process.env.NODE_ENV === 'production' ? 'false' : 'true') === 'true'

synchronize: config.get('DB_SYNCHRONIZE', process.env.NODE_ENV === 'production' ? 'false' : 'true') === 'true',


## apps/server/src/archive/archive-report.attendance.ts（1）
- **[bug] L118-118** Session label mismatch causes the entire detail matrix to render empty. The stored `session` values are English keys — `morning_reading`/`morning`/`afternoon`/`evening_study`/`night_check` (see `@IsIn` in attendance.dto.ts and `mapLessonScheduleTimeToSession` in attendance-time.ts) — but here the table columns are hardcoded Chinese labels `['上午','下午','晚自习']`. `cellMap` is keyed by `r.session`, so `cellMap.get(session)` never matches and every cell always shows `-`, silently dropping all detail data from the report. Additionally the system defines 5 session slots while only 3 columns are rendered. Suggest mapping the real session keys to display labels (covering all 5), e.g. build the columns from `{ morning_reading: '早读', morning: '上午', afternoon: '下午', evening_study: '晚自习', night_check: '晚查' }` or from the actual keys present in the records.

## apps/server/src/archive/archive.service.ts（1）
- **[security] L300-301** IDOR risk: this method (like deleteEnrollment/updateEnrollment/updateExamScore/deleteExamScore/purge* etc.) operates only by `id` without verifying the record belongs to the current student. Note getAttachmentFile does scope by `studentId`, so the API surface is inconsistent. If access is only role-based, any authenticated student can archive/delete other students' records by enumerating ids. Scope the query with the caller's student id (e.g., `{ id, studentId }`) or enforce an explicit ownership check.

## apps/server/src/attendance/attendance-calendar.service.ts（1）
- **[bug] L33-36** Off-by-one weekday bug: `new Date(`${date}T00:00:00+08:00`)` converts China-midnight to an instant whose UTC time is 8 hours earlier (the previous calendar day), and `.getUTCDay()` then returns the weekday of that previous day. E.g. `2026-08-10` (Monday in China) becomes `2026-08-09T16:00Z` (Sunday) and this method returns `7` instead of `1`. As a result `getScheduleOptionsForAttendance` always queries the wrong `cs.weekDay` and returns schedules for the incorrect weekday. Use dayjs (with the same +08 offset used elsewhere in this file) so `.day()` is evaluated in the China timezone.

private getWeekDayForDate(date: string): number { const day = dayjs(${date}T00:00:00+08:00).day(); return day === 0 ? 7 : day; }


## apps/server/src/attendance/attendance-import.service.ts（1）
- **[bug] L87-92** The safety timeout can corrupt the single-import guarantee. If the timer fires while the original import is still genuinely running (which is the only case where it can fire, since `finally` always clears it on the normal path), it sets `isRunning = false` but does nothing to stop the hung import. A second import can then start concurrently, and both share the same `progressSubject` / `importingUserId`, so: (1) the old import's later `emit` calls get tagged with the new user's ID, breaking SSE scoping; (2) when the old import finally exits, its `finally` block resets `isRunning = false` and `importingUserId = undefined` while the new import is still running, allowing a third import to start. Consider using a per-run token/generation counter so only the matching run resets the flags, and refuse starting a new import until the previous run has truly finished.

## apps/server/src/attendance/attendance-import.controller.ts（1）
- **[security] L35-40** Missing class-scope authorization for match/auto-match. Unlike `getDingRaw` (assertClassAccess) and `importFromDingTalk` (getTeacherClassDingUserIds), neither `matchDingRecord` nor `autoMatch` verifies that the raw record/student belongs to the requester's accessible classes. The underlying `AttendanceRecordMutationService.matchDingRecord` simply loads the record by id and saves the match, and `autoMatchDingRecords` iterates ALL unmatched records globally. A user holding only `attendance:edit` (e.g., a teacher) can match records belonging to other classes by guessing IDs and trigger a global auto-match. Suggest resolving the raw record's class and calling `assertClassAccess` before matching, and scoping `autoMatch` to the user's accessible classes (or requiring `canManageAllAttendance`).

## apps/server/src/attendance/attendance-report.service.ts（1）
- **[bug] L179-181** The "consecutive" absence/late detection does not actually check that dates are consecutive calendar days. The query only returns records with status IN ('absent','late'), so any intervening present/leave/pending record is invisible — a student absent Mon, present Tue, absent Wed will be counted as count=2 "consecutive" absences. Additionally, multiple sessions on the same day (morning/afternoon) are counted as separate days, and the lastDate update (`r.attendanceDate > current.lastDate`) never fires because the list is already sorted by date DESC. To correctly detect consecutive absences, group records per student per day (dedupe by date) and verify date difference === 1 day between consecutive entries.

## apps/server/src/auth/auth.module.ts（1）
- **[security] L20-20** Security: the JWT signing secret falls back to a hardcoded value `'dorm-billing-jwt-secret-key-2024'` when the `JWT_SECRET` env var is missing. This exact value is publicly documented in the repo's technical doc/.env.example, so if the env var is ever unset (e.g., misconfigured deployment, local/test env), anyone who knows the source can forge valid JWTs and impersonate any user (including super admins). It also silently degrades security instead of failing fast. Recommend removing the fallback and throwing when `JWT_SECRET` is absent (e.g., validate at startup), and ideally validating the secret length/strength. Same fallback also appears in jwt.strategy.ts and should be aligned.

secret: (() => { const secret = config.get('JWT_SECRET'); if (!secret) { throw new Error('JWT_SECRET environment variable is required but not set'); } return secret; })(),


## apps/server/src/auth/dto/auth.dto.ts（1）
- **[security] L7-9** Password minimum length of 4 is far too weak for a credential (allows trivial brute-force, e.g., "abcd"). Also, without a @MaxLength bound, an attacker can submit an arbitrarily large password, forcing expensive hashing on the server (DoS vector). Recommend @MinLength(8) plus a reasonable @MaxLength, and ideally a complexity rule (e.g., @Matches for letter/digit mix).

@IsString() @MinLength(8) @MaxLength(128) password: string;


## apps/server/src/auth/strategies/jwt.strategy.ts（1）
- **[security] L30-30** Security: the JWT secret falls back to a hardcoded, publicly committed value ('dorm-billing-jwt-secret-key-2024'). If JWT_SECRET is not set in the environment, tokens are signed/verified with this known secret, letting anyone forge a JWT with an arbitrary `sub` and impersonate any user (full auth bypass). This is also a predictable, short key. Remove the default and fail fast at startup when JWT_SECRET is missing, e.g. use config.getOrThrow('JWT_SECRET').

  secretOrKey: config.getOrThrow<string>('JWT_SECRET'),


## apps/server/src/bills/bills.controller.ts（1）
- **[bug] L122-123** Route shadowing bug: `@Put('batch/status')` is declared after `@Put(':id/status')`, so a request `PUT /bills/batch/status` is matched by the `:id/status` route first with `id='batch'`, and `ParseIntPipe` throws a 400 Bad Request — `batchUpdateStatus` is effectively unreachable via HTTP. Move the `batch/status` route above `:id/status` (static path before param path) to fix.

## apps/server/src/bills/bills-generation.service.ts（1）
- **[bug] L45-46** `longTermOccupancies` is initialized as an empty array and never populated, so the `.filter((o) => o.stayType === 'long')` in the `roomIds` computation below always yields nothing. Consequently, rooms that only have long-term occupants (and no active RoomExpense within the period) are never added to `roomIds`, and the long-term rent block inside the per-room loop never runs for them — those students silently get no rent bill. Either populate this array (e.g. query long-term occupancies overlapping the period via `occRepo`) or remove the dead variable and its use in `roomIds`.

## apps/server/src/classroom-rentals/classroom-rentals.controller.ts（1）
- **[bug] L205-206** `fs.createReadStream(fullPath)` is piped to the response without any error handler. If the contract file is missing (e.g., deleted between `getContractPath` and streaming) or a read error occurs, the ReadStream emits an 'error' event with no listener, which throws an unhandled 'error' event and can crash the whole Node process (or leave the client hanging). Also, `Content-Length` is not set. Attach an error listener and destroy the response appropriately, e.g.:

```ts
const stream = fs.createReadStream(fullPath);
stream.on('error', (err) => {
if (!res.headersSent) res.status(500).json({ message: '合同文件读取失败' });
else res.destroy(err);
});
stream.pipe(res);

apps/server/src/classrooms/classrooms.controller.ts（1）

[bug] L209-211 Static route is shadowed by the parameterized route: @Get(':id') is declared before @Get('report'), so a request to GET /classrooms/report is matched by findOne('report') first, where +('report') evaluates to NaN (likely causing a DB error / 500) and the report endpoint becomes unreachable. Move @Get('report') (and any other static route) above @Get(':id'), as already done for @Get('template'). Additionally, consider rejecting non-numeric :id values in findOne to harden the param route.

apps/server/src/classroom-rentals/dto/rental.dto.ts（1）

[bug] L16-20 No cross-field validation guarantees that endDate is on/after startDate. A request with endDate earlier than startDate (or equal, if zero-length periods are invalid) passes validation and can create an invalid rental period. The same gap exists in UpdateRentalDto. Add a class-level custom validator (e.g., a ValidatorConstraint or a @ValidateIf/@IsAfter combination) that compares the two fields for both DTOs.

apps/server/src/common/request-utils.ts（1）

[security] L11-15 Client IP is derived by blindly trusting the x-forwarded-for/x-real-ip headers. These headers are client-controlled unless the server is only reachable behind a trusted reverse proxy that overwrites them. If the service can be hit directly, an attacker can spoof any IP with X-Forwarded-For: <victim-ip>, bypassing IP-based rate limiting / access control and poisoning audit logs. Suggest honoring these headers only when the direct peer (req.connection.remoteAddress) is a known trusted proxy (or relying on the framework's trust proxy setting), and falling back to the connection address otherwise.

apps/server/src/common/with-audit-log.ts（1）

[bug] L35-36 If logService.log() throws (e.g. DB failure) after operation() has already succeeded, the exception propagates and the caller sees a failure even though the business operation was executed — it may retry the operation and produce duplicate side effects. Audit logging should not be able to fail the business operation; wrap the log call in try/catch and log the audit error instead of re-throwing.


## apps/server/src/classrooms/classrooms.service.ts（1）
- **[bug] L399-406** Usage report overcounts schedule days: `ClassSchedule` is a weekly recurring rule (single `weekDay` 1–7 spanning `startDate`–`endDate`, as used in `getUsageForClassrooms`), but this loop counts *every* calendar day in the range. A Monday-only weekly class spanning a month is counted as ~29 days instead of ~4-5, inflating `scheduleDays`/`usedDays` and `occupancyRate`. Only count days whose `getDay()` matches `s.weekDay` (JS: 0=Sun..6=Sat, so `target = s.weekDay % 7`).

for (const s of schedules) {
  if (!scheduleDaysByRoom[s.classroomId]) scheduleDaysByRoom[s.classroomId] = new Set();
  const effStart = new Date(Math.max(new Date(s.startDate).getTime(), start.getTime()));
  const effEnd = new Date(Math.min(new Date(s.endDate).getTime(), end.getTime()));
  const targetJsDay = s.weekDay % 7; // weekDay: 1=Mon..7=Sun -> JS getDay(): 0=Sun..6=Sat
  for (let d = new Date(effStart); d <= effEnd; d.setDate(d.getDate() + 1)) {
    if (d.getDay() !== targetJsDay) continue;
    scheduleDaysByRoom[s.classroomId].add(dayjs(d).utcOffset(8).format('YYYY-MM-DD'));
  }
}


## apps/server/src/deposits/deposits.service.ts（1）
- **[bug] L239-240** Potential data loss: `findOne` here matches ANY deposit of the student, including archived ones. Because `purge()` refuses to permanently delete deposits that carry `refundAmount > 0`, archived records with refund history persist. A subsequent `create`/`batchCreate` for that student will then reopen the archived record, overwrite `status` to 'paid', and null out `refundDate/refundAmount/refundedBy/refundedAt` — permanently erasing the historical refund/audit trail. Additionally this check-then-act (findOne followed by save/insert) is not atomic; two concurrent creates for the same new student can both pass the `!existing` branch and produce duplicate deposit rows. Suggest excluding archived records (e.g. `where: { studentId, status: Not('archived') }` and import `Not` from typeorm), or always inserting a fresh record when an archived one exists.

const existing = await this.repo.findOne({
  where: { studentId: dto.studentId, status: Not('archived') },
});
if (existing) {


## apps/server/src/entities/class-student.entity.ts（1）
- **[bug] L29-31** Inconsistent referential behavior: the `class` relation uses `onDelete: 'CASCADE'`, but the `student` relation has no `onDelete` option (defaults to NO ACTION/RESTRICT at the DB level). Since `student_id` is NOT NULL, deleting a Student that still has rows in `class_student` will throw a foreign-key constraint error at runtime, while deleting a Class silently cascades. Either add `onDelete: 'CASCADE'` here (appropriate for a join table) or align the behavior deliberately (e.g., soft-delete students).

@ManyToOne(() => Student, { onDelete: 'CASCADE' }) @JoinColumn({ name: 'student_id' }) student: Student;


## apps/server/src/entities/bill.entity.ts（1）
- **[bug] L27-28** TypeORM's MySQL driver returns DECIMAL columns as strings at runtime, not numbers. These money fields (sharedAmount, personalAmount, totalAmount, paidAmount, outstandingAmount) are declared as `number`, so any arithmetic like `paidAmount + outstandingAmount` or `total - paid` will silently do string concatenation (e.g. "10" + "5" = "105") and JSON serialization will yield strings. Add a value transformer (e.g. a shared ColumnNumericTransformer with to/from: Number) to all five decimal columns, or declare them as `string` if kept raw. Note the same pattern exists elsewhere in the codebase, so a shared transformer is advisable.

@Column({ name: 'shared_amount', type: 'decimal', precision: 10, scale: 2, default: 0, transformer: numericTransformer }) sharedAmount: number;


## apps/server/src/entities/learning-record.entity.ts（1）
- **[bug] L17-18** `student_id` is mapped twice: once by the raw `@Column` (`studentId`) and once by the `@ManyToOne` + `@JoinColumn` relation (`student`). When saving a LearningRecord, TypeORM will include the `student_id` column for both properties, which can produce an SQL error ("column student_id specified more than once") or cause the relation FK to be written unexpectedly. Keep only the relation and access the FK via `record.student.id`, or if a raw accessor is genuinely needed, declare it as `@Column({ name: 'student_id', insert: false, update: false })` and let the relation own writes.

## apps/server/src/entities/student-wallet.entity.ts（1）
- **[bug] L20-21** Type mismatch: DECIMAL columns are returned as strings by the MySQL/Postgres drivers, so `balance` will actually be a `string` at runtime even though it's typed `number`. This can cause silent bugs like string concatenation instead of arithmetic (e.g., `balance + amount`). Either declare the property as `string`, or add a transformer that converts the DB value to a number:

```ts
@Column({
type: 'decimal',
precision: 12,
scale: 2,
default: 0,
transformer: {
  to: (v: number) => v,
  from: (v: string) => parseFloat(v),
},
})
balance: number;

apps/server/src/entities/student-profile.entity.ts（1）

[bug] L17-18 The student_id database column is mapped twice: once by the @Column property studentId and once by the @JoinColumn({ name: 'student_id' }) of the student relation. TypeORM will register two column metadata entries for the same physical column, so an INSERT (e.g. profileRepo.create({ ...dto, studentId }) in archive.service) can emit the column twice in the SQL (INSERT INTO student_profiles (student_id, ..., student_id) ...), causing MySQL error 1110 "Column 'student_id' specified twice" or ambiguous mapping at runtime. Keep only the relation (with @JoinColumn) and expose the FK id via @RelationId, or drop the relation and keep only the @Column.
```
@RelationId((profile: StudentProfile) => profile.student)
studentId: number;
```

apps/server/src/expense-types/expense-types.service.ts（1）

[bug] L55-57 remove() soft-deletes a type by setting enabled=false, but this uniqueness check matches soft-deleted rows as well. As a result, re-creating a previously removed type with the same code always throws ConflictException, and seedDefaults() will never restore a disabled default type either. Scope the uniqueness check to enabled records only (and consider re-enabling soft-deleted defaults in seedDefaults).
```
  const normalized = { ...dto, code: dto.code.trim(), name: dto.name.trim() };
  const exists = await this.repo.findOne({ where: { code: normalized.code, enabled: true } });
  if (exists) throw new ConflictException('费用类型代码已存在');
```

apps/server/src/financial-operations/financial-operations.service.ts（1）

[bug] L35-40 Non-atomic retry claim can cause duplicate execution. When an existing operation is 'failed' (or any non-completed/non-running state), the transition to 'running' is a read-modify-write (findOne → save) with no locking. Two concurrent retry requests can both read status='failed', both save 'running', and both execute work() — for financial operations this can produce duplicate payments/refunds. Use an atomic conditional claim (UPDATE ... WHERE status='failed' and check affected rows) or a pessimistic write lock within a transaction so only one request claims the operation; also re-fetch the row after claiming so the later status updates use fresh data.
```
  } else {
    const claim = await this.repo.update(
      { operationId, status: 'failed' },
      { status: 'running', errorMessage: null, resultJson: null },
    );
    if (!claim.affected) {
      const fresh = await this.repo.findOne({ where: { operationId } });
      if (fresh?.status === 'completed' && fresh.resultJson) return JSON.parse(fresh.resultJson) as T;
      throw new ConflictException('该操作正在处理中，请勿重复提交');
    }
    operation = (await this.repo.findOne({ where: { operationId } }))!;
  }
```

apps/server/src/imports/imports.commit.service.ts（1）

[bug] L102-105 Concurrency hazard: the run/step transition to 'committing' is a plain save outside any transaction, lock, or optimistic-version column (no @Version exists in the module). Two concurrent commitStep calls for the same run can both pass the earlier status checks (both see ready) and both run the write transaction, producing duplicate created/updated records. Use an atomic conditional update (e.g. UPDATE ... WHERE status='ready' and check affected rows) or a pessimistic/optimistic lock around the state transition.

apps/server/src/imports/imports.workbook-fallback.ts（1）

[bug] L109-109 A self-closing empty cell (<c r="B1"/>, emitted by several xlsx writers) is not matched because this regex requires a closing </c>. When a self-closing cell sits between populated cells, matchAll pairs <c r="B1"/> with the next cell's </c>, so the following cell's value is parsed with the wrong column reference — data shifts left and trailing cells are lost. Handle the self-closing form explicitly (e.g. match |<c\b([^>]*)\/> as an empty-cell alternative and adjust group indexes), or split the row body on <c\b and parse each segment.

apps/server/src/integration/entities/integration-config.entity.ts（1）

[security] L66-67 Security: this column stores sensitive third-party credentials (corpId/appSecret, possibly in plaintext per the class comment). Storing secrets in plaintext in the DB and exposing content directly in API responses/logs risks credential leakage. Recommend encrypting sensitive fields at rest, masking them in responses/logs, and ensuring content is never returned to clients without sanitization.

apps/server/src/migration-runner.ts（1）

[bug] L43-45 If ds.initialize() or ds.runMigrations() throws, ds.destroy() is never called, leaking the MySQL connection and potentially leaving the process hanging. Wrap the migration steps in try/catch/finally so the data source is always destroyed (guarding with ds.isInitialized), and rethrow/log the error so the caller can fail fast.
```
try {
  await ds.initialize();
  await ds.runMigrations();
} catch (err) {
  console.error('Failed to run database migrations on startup', err);
  throw err;
} finally {
  if (ds.isInitialized) {
    await ds.destroy();
  }
}
```

apps/server/src/migrations/1784520727860-InitialSchema.ts（1）

[bug] L227-228 The down() migration drops the same index IDX_050485162d4fe47bd3cfd7dedb on room_expenses twice in a row. The first statement succeeds and the second fails with MySQL error 1091 (index does not exist), aborting the entire rollback and leaving the database in a partially migrated state. Remove the duplicated statement.
```
      await queryRunner.query(`DROP INDEX \`IDX_050485162d4fe47bd3cfd7dedb\` ON \`room_expenses\``);
```

apps/server/src/occupancies/occupancies.service.ts（1）

[bug] L75-80 Concurrency hazard: the check for an existing active occupancy uses SELECT ... FOR UPDATE on a row that does not yet exist (phantom read), so it acquires no lock. Two concurrent check-ins for the same student (e.g., in different rooms) can both pass this check, create two active occupancy records, and double-collect the deposit (the deposit row is also read without a lock, causing lost updates). Fix by locking a stable row (e.g., SELECT ... FOR UPDATE on the Student row before this check) or by adding a DB-level unique partial index on (student_id) WHERE check_out_date IS NULL.

apps/server/src/occupancies/occupancies.controller.ts（1）

[security] L94-96 DTO validation is not enforced on these mutation routes. Only batchRestore applies @UsePipes(new ValidationPipe(...)), and there is no global ValidationPipe registered (main.ts / APP_PIPE don't register one), so the class-validator decorators on CheckInDto, CheckOutDto, TransferRoomDto and BatchCheckOutDto (required fields, @IsInt, @IsISO8601, @Min, etc.) are never executed. Malformed or extra fields reach the service and DB unchecked. Apply the same pipe at the controller level (or on each route).
```
@Post('check-in')
@RequirePermission('occupancy:checkin')
@UsePipes(new ValidationPipe({ transform: true, whitelist: true, forbidNonWhitelisted: true }))
async checkIn(@Body() dto: CheckInDto, @Request() req: AuthenticatedRequest) {
```

apps/server/src/occupancies/occupancy-import.service.ts（1）

[bug] L139-139 isHistoricalRecord is defined solely by the presence of checkOutDate, so any row with a future (planned) check-out date is treated as a historical record. That bypasses the active-occupancy conflict check, the room-capacity check, bed/locker status updates, and deposit collection — a live/current check-in with an end date can over-fill a room and leave the bed/locker marked 'available'. Historical should mean the check-out date is in the past, e.g. checkOutDate < today.

const today = dayjs().utcOffset(8).format('YYYY-MM-DD'); const isHistoricalRecord = Boolean(checkOutDate && checkOutDate < today);


## apps/server/src/occupancies/occupancy-operations.service.ts（1）
- **[bug] L76-78** Unconditionally setting the room to 'available' is incorrect for multi-occupancy rooms (capacity > 1 is clearly supported here — see the capacity/count logic in transferRoom). After one resident checks out, other active occupancies may remain, so the room should only become 'available' when no active occupancy is left; otherwise it should stay 'occupied'. It also overwrites an existing 'maintenance' status. Please count remaining active occupancies (checkOutDate IS NULL) before deciding the new status.

## apps/server/src/rbac/rbac-user.service.ts（1）
- **[security] L266-267** `updateTeacherProfile` returns the full `User` entity directly from `save()`. The entity contains sensitive fields such as `passwordHash` (and any other internal columns), which would be exposed in the API response if the controller returns it as-is. All other methods in this service return a sanitized shape (`{ message, profile }`). Return a DTO/sanitized object instead.

user.profile = { ...user.profile, ...profile };
await this.userRepo.save(user);
return { message: '资料已更新', profile: user.profile };


## apps/server/src/rbac/rbac-seed.service.ts（1）
- **[bug] L142-142** `seedData()` is invoked on every application startup (rbac.module.ts `onModuleInit`). This bulk UPDATE unconditionally re-activates ALL users with `isActive = false` on every boot, regardless of why they were disabled. Other code paths still write this field (e.g. `restoreUser` explicitly sets `isActive: true`), so any account set inactive — deliberately or by legacy/integration code — is silently re-enabled at the next restart. If this is meant as a one-time legacy migration, guard it with a migration/version flag or narrow the criteria so intentionally disabled accounts are not resurrected on every deploy.

## apps/server/src/schedules/schedules.controller.ts（1）
- **[security] L90-90** Authorization bypass: in schedules.service.ts, `findAll` only applies the `accessibleClassIds` restriction in the `else if` branch — when `query.classId` is provided, the accessible-class filter is skipped entirely. A teacher with only `schedule:view` can therefore pass an arbitrary `classId` and read schedules of classes they are not assigned to. Validate that `query.classId` is within the computed `classIds` here (and similarly for other handlers that forward a `classId` filter).

if (query.classId !== undefined && classIds && !classIds.includes(query.classId)) {
  return [];
}
return this.service.findAll(query, classIds);


## apps/server/src/students/students.import.service.ts（1）
- **[bug] L44-48** `batchImport` performs multiple DB writes per row (existence check, student insert, archive imports) with no try/catch and no transaction. A single bad row (e.g., unique-constraint violation on studentNo/phone/idNumber, or an invalid archive row) aborts the entire import with a raw DB error, while rows already processed remain partially committed. Consider wrapping the import in a transaction and handling per-row errors (collect failures and continue) so one bad row doesn't discard the whole workbook.

## apps/server/src/schedules/schedules.service.ts（1）
- **[security] L103-104** Authorization bypass in `findAll`: when a caller supplies `query.classId`, the `accessibleClassIds` scope is skipped entirely (the `else if` branch never runs). The controller passes the teacher's accessible class IDs straight through without validating that `query.classId` belongs to the user, so a restricted teacher can list schedules of any class by passing its id — unlike `schedule-queries.service.ts` which rejects `classId` outside the accessible set. Enforce the intersection here, e.g. return `[]` when `accessibleClassIds` is defined and does not contain `query.classId`.

if (query.classId) {
  if (accessibleClassIds && !accessibleClassIds.includes(query.classId)) return [];
  qb.andWhere('cs.classId = :classId', { classId: query.classId });
} else if (accessibleClassIds) {


## apps/server/src/sync/dto/schedule-sync.dto.ts（1）
- **[bug] L6-8** Contradictory validation rules make `dateFrom` impossible to validate successfully. `@Matches(/^\d{4}-\d{2}-\d{2}$/)` only accepts a bare `YYYY-MM-DD` date, but `@IsISO8601({ strict: true })` (validator.js strict mode) requires a full ISO 8601 timestamp with the `T` separator, time portion, and optional timezone (e.g. `2026-08-09T00:00:00.000Z`). Since class-validator requires all constraints on a property to pass, no value can ever satisfy both — any request that includes `dateFrom` will be rejected with a 400, making the query parameter unusable. Fix by picking one format: drop `strict: true` so `@IsISO8601()` accepts plain dates, or align the regex to the strict ISO format.

@Matches(/^\d{4}-\d{2}-\d{2}$/) @IsISO8601() dateFrom?: string;


## apps/server/src/students/students.service.ts（1）
- **[security] L110-116** Authorization gap: unlike `findAll` and `getFilterLookups`, this method accepts no `accessibleClassIds` and the controller (`GET /students/basic-lookups`, permission `student:basic-view`/`student:view`) calls it without class scoping. As a result, any teacher who can view students gets every active student's PII (phone, studentNo, gender) across all classes — a horizontal privilege escalation inconsistent with the class-scoped behavior elsewhere in this file. Suggest accepting `accessibleClassIds` and filtering via `classStudentRepo` (same as `findAll`), or restricting this endpoint to manage-all users.

async getBasicLookups(accessibleClassIds?: number[]) { const where: FindOptionsWhere = { status: 'active' }; if (accessibleClassIds) { const classStudents = await this.classStudentRepo.find({ where: { classId: In(accessibleClassIds), status: 'active' }, }); where.id = In([...new Set(classStudents.map((item) => item.studentId))]); } return this.repo.find({ select: ['id', 'name', 'studentNo', 'gender', 'phone', 'status'], where, order: { name: 'ASC' }, }); }


## apps/server/src/sync/schedule-sync.service.ts（1）
- **[bug] L206-207** Attendance group name is derived only from className. If two different classes share the same name (very common in schools, e.g. "高一(1)班" in different grades/years), the second class will hit the group cached under the same name and `updateAttendanceGroup` will overwrite the group's members and shift_ids with the second class's data. The first class's already-written schedule items then point to a group whose members were replaced, corrupting attendance data. Unlike shift names, the group name has no period component to disambiguate. Suggest appending the classId: `排课_${className}_${classId}`.

  // 创建/匹配该班级的考勤组
  const groupName = `排课_${className}_${classId}`;


## apps/server/src/sync/sync-runner.ts（1）
- **[bug] L43-45** Errors thrown by `releaseLease` in the `finally` block will mask the original result: if the operation succeeded (and the log/state were updated) but releasing the lease fails (e.g. a transient DB error), the caller receives an exception for a fully successful sync, and the lease stays held until it goes stale. Wrap the release in a try/catch and only log the failure instead of rethrowing.

} finally {
  try {
    await this.releaseLease(platform, runId);
  } catch (releaseError) {
    this.logger.error(`Failed to release lease for ${platform}: ${releaseError}`);
  }
}


## apps/server/src/sync/sync.service.ts（1）
- **[bug] L223-225** When a Jinshuju entry has all mapped fields empty, `mappedValues` becomes `{}` and `manager.update(Student, decision.matchStudentId, {})` throws TypeORM's `UpdateValuesMissingError` ("Cannot perform update query because update values are not defined"), which rolls back the entire jinshuju sync transaction and fails the whole sync. Guard the update with a non-empty check (or skip the entry) before calling `update`.

      if (decision.action === 'match' && decision.matchStudentId) {
        if (Object.keys(mappedValues).length > 0) {
          await manager.update(Student, decision.matchStudentId, mappedValues);
          matched++;
        }


## apps/server/src/wallets/wallets.service.ts（1）
- **[bug] L200-201** Concurrency / double-spend risk: `debitBill` (and `refundBill`/`settleOutstandingBills`) read `wallet.balance` with `getOrCreateWallet(manager, bill.studentId)` (lock defaults to false → plain `findOne`, no `FOR UPDATE`). External callers (`bills.service.ts`, `bills-generation.service.ts`) invoke `debitBill` inside their own transactions without ever locking the wallet row. In MySQL/InnoDB a plain SELECT is a non-blocking consistent read, so two concurrent debits for the same student (e.g. two bills generated in parallel, or a debit racing a wallet adjustment) can both read the same balance and both apply it → lost update / balance below zero / bill marked paid on money already spent. Since all callers pass a transactional `EntityManager`, read the wallet with `setLock('pessimistic_write')` (pass lock=true) in `debitBill`/`refundBill`, or use an atomic conditional update (`UPDATE student_wallets SET balance = balance - x WHERE student_id = ? AND balance >= x`).

const wallet = await this.getOrCreateWallet(manager, bill.studentId, true);
const amount = money(Math.min(Math.max(0, money(wallet.balance)), remaining));


## serve-proxy.js（1）
- **[bug] L40-46** `API_TARGET` is read from the environment but never actually used — the proxy hardcodes `127.0.0.1:3000` (hostname, port, and host header are all hardcoded in three places). If `API_TARGET` points to a different host/port, requests are still forwarded to 127.0.0.1:3000, silently breaking the configuration. Parse the target once with `new URL(API_TARGET)` and derive hostname/port/host from it (note `http.request` also can't handle an `https://` target).

const target = new URL(API_TARGET);
const opts = {
  hostname: target.hostname,
  port: target.port || (target.protocol === 'https:' ? 443 : 80),
  path: (target.pathname === '/' ? '' : target.pathname) + req.url,
  method: req.method,
  headers: { ...req.headers, host: target.host },
};

64 KiB Raw Blame History Unescape Escape