Repository navigation
Migrate PetrikWiFi into Filc - #390
Legolaszstudio wants to merge 63 commits into
Conversation
Sync 2026.08.30. 21:30
Sync 2026. 09. 01. 12:00
# Conflicts: # .gitignore # apps/chronos/.env.example # apps/chronos/src/database/migrations/meta/0018_snapshot.json # apps/chronos/src/database/migrations/meta/_journal.json # apps/chronos/src/index.ts # apps/chronos/src/utils/environment.ts # apps/iris/public/locales/en/translation.json # apps/iris/public/locales/hu/translation.json # apps/iris/src/components/admin/sidebar.tsx # apps/iris/src/route-tree.gen.ts # apps/iris/src/routes/_private/route.tsx # apps/iris/src/routes/auth/welcome.tsx # packages/ui/src/components/password-input.tsx
… filc, auto attach wifi account upon registration
|
Important
This repository does not receive automatic reviews because it has fewer than 10 stars. ⚙️ Run configurationConfiguration used: Organization UI Review profile: ASSERTIVE Plan: Advanced Run ID: 📝 WalkthroughWalkthroughChangesThe pull request adds a complete WiFi platform. Chronos gains WiFi storage, APIs, RADIUS authorization, UniFi integration, scheduled jobs, and migration support. Iris gains self-service and administration pages. FreeRADIUS deployment files and certificate tooling are added. WiFi platform
Priority: ➖ Normal Merge Risk: 🟠 High · up to Enabling WiFi can expose credentials, assign legacy accounts to the wrong user, interrupt authentication, or leave migrations and integration data incomplete. Keep the feature disabled until these issues are fixed. 🚥 Pre-merge checks | ✅ 4✅ Passed checks (4 passed)
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 |
|
És most megyek aludni, jó éjszakát! 🌙 |
|
@CodeRabbit review nyulhusleves |
|
✅ Action performedReview finished.
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 31
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: ASSERTIVE
Plan: Advanced
Run ID: 7692e0c4-9be4-46fa-a957-1edd766e6988
⛔ Files ignored due to path filters (2)
apps/radius/certs/ca.pemis excluded by!**/*.pembun.lockis excluded by!**/*.lock
📒 Files selected for processing (81)
apps/chronos/.env.exampleapps/chronos/scripts/migrate-petrikwifi.tsapps/chronos/src/_types/hc.tsapps/chronos/src/database/index.tsapps/chronos/src/database/migrations/0022_wifi_tables.sqlapps/chronos/src/database/migrations/0023_wide_midnight.sqlapps/chronos/src/database/migrations/meta/0022_snapshot.jsonapps/chronos/src/database/migrations/meta/0023_snapshot.jsonapps/chronos/src/database/migrations/meta/_journal.jsonapps/chronos/src/database/schema/wifi.tsapps/chronos/src/database/scripts/import-navigator.tsapps/chronos/src/database/scripts/reset.tsapps/chronos/src/database/scripts/seed.tsapps/chronos/src/index.tsapps/chronos/src/routes/features/index.tsapps/chronos/src/routes/wifi/_factory.tsapps/chronos/src/routes/wifi/_router.tsapps/chronos/src/routes/wifi/admin.tsapps/chronos/src/routes/wifi/radius.tsapps/chronos/src/routes/wifi/self.tsapps/chronos/src/routes/wifi/stats.tsapps/chronos/src/routes/wifi/status.tsapps/chronos/src/utils/authentication.tsapps/chronos/src/utils/authorization.tsapps/chronos/src/utils/cron.tsapps/chronos/src/utils/environment.tsapps/chronos/src/utils/wifi/cleanup.tsapps/chronos/src/utils/wifi/constants.tsapps/chronos/src/utils/wifi/controller.tsapps/chronos/src/utils/wifi/encryptor.tsapps/chronos/src/utils/wifi/mac.tsapps/chronos/src/utils/wifi/radius.tsapps/chronos/src/utils/wifi/speed-profile.tsapps/chronos/src/utils/wifi/sync.tsapps/chronos/src/utils/wifi/unifi-api.tsapps/chronos/src/utils/wifi/unifi-controller.tsapps/chronos/src/utils/wifi/unifi-helpers.tsapps/iris/package.jsonapps/iris/public/locales/en/translation.jsonapps/iris/public/locales/hu/translation.jsonapps/iris/src/components/admin/sidebar.tsxapps/iris/src/components/navbar.tsxapps/iris/src/components/wifi/relative-time.tsxapps/iris/src/components/wifi/wifi-auth-chart.tsxapps/iris/src/components/wifi/wifi-dialogs.tsxapps/iris/src/components/wifi/wifi-filters.tsxapps/iris/src/hooks/wifi-admin.tsapps/iris/src/hooks/wifi.tsapps/iris/src/route-tree.gen.tsapps/iris/src/routes/_private/admin/wifi/index.tsxapps/iris/src/routes/_private/admin/wifi/nas.tsxapps/iris/src/routes/_private/admin/wifi/speed-profiles.tsxapps/iris/src/routes/_private/admin/wifi/users.tsxapps/iris/src/routes/_private/route.tsxapps/iris/src/routes/_private/wifi.tsxapps/iris/src/routes/auth/welcome.tsxapps/iris/src/utils/hc.tsapps/iris/src/utils/query-keys.tsapps/radius/.env.exampleapps/radius/certs/.gitignoreapps/radius/certs/Makefileapps/radius/certs/ca.crlapps/radius/certs/ca.derapps/radius/certs/passwords.example.mkapps/radius/docker-compose.ymlapps/radius/freeradius/clients.confapps/radius/freeradius/defaultapps/radius/freeradius/docker-entrypoint.shapps/radius/freeradius/eapapps/radius/freeradius/eapol_test.confapps/radius/freeradius/inner-eapapps/radius/freeradius/inner-tunnelapps/radius/freeradius/restbiome.jsoncdocs/freeradius-setup.mdpackages/api/src/domains/wifi/admin.tspackages/api/src/domains/wifi/radius.tspackages/api/src/domains/wifi/self.tspackages/api/src/domains/wifi/stats.tspackages/api/src/permissions.tspackages/ui/src/components/password-input.tsx
💤 Files with no reviewable changes (2)
- apps/chronos/src/database/scripts/seed.ts
- apps/chronos/src/database/scripts/import-navigator.ts
Included review availability: Your plan provides up to 1 included review per hour; 0 remain after this review.
📜 Review details
🧰 Additional context used
📓 Path-based instructions (1)
Domain data access lives in hook modules at [apps/iris/src/hooks](apps/iris/src/hooks)
📄 CodeRabbit inference engine (AGENTS.md)
Files:
apps/iris/src/hooks/wifi-admin.tsapps/iris/src/hooks/wifi.ts
🪛 Betterleaks (1.8.1)
apps/radius/freeradius/eap
[high] 21-21: Detected a potential hardcoded password literal, which may expose account credentials.
(generic-password)
packages/ui/src/components/password-input.tsx
[high] 31-31: Detected a potential hardcoded password literal, which may expose account credentials.
(generic-password)
apps/radius/freeradius/eapol_test.conf
[high] 7-7: Detected a potential hardcoded password literal, which may expose account credentials.
(generic-password)
apps/radius/freeradius/inner-eap
[high] 14-14: Detected a potential hardcoded password literal, which may expose account credentials.
(generic-password)
apps/iris/public/locales/en/translation.json
[high] 66-66: Detected a potential hardcoded password literal, which may expose account credentials.
(generic-password)
[high] 67-67: Detected a potential hardcoded password literal, which may expose account credentials.
(generic-password)
[high] 71-71: Detected a potential hardcoded password literal, which may expose account credentials.
(generic-password)
[high] 193-193: Detected a potential hardcoded password literal, which may expose account credentials.
(generic-password)
[high] 197-197: Detected a potential hardcoded password literal, which may expose account credentials.
(generic-password)
[high] 198-198: Detected a potential hardcoded password literal, which may expose account credentials.
(generic-password)
[high] 199-199: Detected a potential hardcoded password literal, which may expose account credentials.
(generic-password)
[high] 201-201: Detected a potential hardcoded password literal, which may expose account credentials.
(generic-password)
apps/iris/public/locales/hu/translation.json
[high] 71-71: Detected a potential hardcoded password literal, which may expose account credentials.
(generic-password)
[high] 175-175: Detected a potential hardcoded password literal, which may expose account credentials.
(generic-password)
[high] 179-179: Detected a potential hardcoded password literal, which may expose account credentials.
(generic-password)
[high] 180-180: Detected a potential hardcoded password literal, which may expose account credentials.
(generic-password)
[high] 181-181: Detected a potential hardcoded password literal, which may expose account credentials.
(generic-password)
[high] 183-183: Detected a potential hardcoded password literal, which may expose account credentials.
(generic-password)
[high] 342-342: Detected a potential hardcoded password literal, which may expose account credentials.
(generic-password)
[high] 343-343: Detected a potential hardcoded password literal, which may expose account credentials.
(generic-password)
🪛 checkmake (0.3.2)
apps/radius/certs/Makefile
[warning] 48-48: Target body for "passwords.mk" exceeds allowed length of 5 lines (6).
(maxbodylength)
[warning] 69-69: Target body for "ca.key ca.pem" exceeds allowed length of 5 lines (6).
(maxbodylength)
[warning] 158-158: Required target "clean" must be declared PHONY.
(minphony)
[warning] 158-158: Required target "test" is missing from the Makefile.
(minphony)
🪛 dotenv-linter (4.0.0)
apps/radius/.env.example
[warning] 5-5: [UnorderedKey] The RADTEST_PASS key should go before the RADTEST_USER key
(UnorderedKey)
[warning] 6-6: [UnorderedKey] The RADTEST_SECRET key should go before the RADTEST_USER key
(UnorderedKey)
[warning] 8-8: [EndingBlankLine] No blank line at the end of the file
(EndingBlankLine)
🪛 OpenGrep (1.29.0)
apps/chronos/src/utils/wifi/unifi-api.ts
[WARNING] 60-60: TLS certificate verification is globally disabled by setting NODE_TLS_REJECT_UNAUTHORIZED to "0". This allows man-in-the-middle attacks on all HTTPS connections in the process.
(coderabbit.tls.node-tls-reject-unauthorized)
🪛 Squawk (2.64.0)
apps/chronos/src/database/migrations/0023_wide_midnight.sql
[warning] 1-1: Dropping a NOT NULL constraint may break existing clients.
(ban-drop-not-null)
[warning] 2-2: By default new constraints require a table scan and block writes to the table while that scan occurs. Use NOT VALID with a later VALIDATE CONSTRAINT call.
(constraint-missing-not-valid)
[warning] 2-2: Adding a foreign key constraint requires a table scan and a SHARE ROW EXCLUSIVE lock on both tables, which blocks writes to each table. Add NOT VALID to the constraint in one transaction and then VALIDATE the constraint in a separate transaction.
(adding-foreign-key-constraint)
apps/chronos/src/database/migrations/0022_wifi_tables.sql
[warning] 3-3: Using 32-bit integer fields can result in hitting the max int limit. Use 64-bit integer values instead to prevent hitting this limit.
(prefer-bigint-over-int)
[warning] 8-8: When Postgres stores a datetime in a timestamp field, Postgres drops the UTC offset. This means 2019-10-11 21:11:24+02 and 2019-10-11 21:11:24-06 will both be stored as 2019-10-11 21:11:24 in the database, even though they are eight hours apart in time. Use timestamptz instead of timestamp for your column type.
(prefer-timestamp-tz)
[warning] 17-17: When Postgres stores a datetime in a timestamp field, Postgres drops the UTC offset. This means 2019-10-11 21:11:24+02 and 2019-10-11 21:11:24-06 will both be stored as 2019-10-11 21:11:24 in the database, even though they are eight hours apart in time. Use timestamptz instead of timestamp for your column type.
(prefer-timestamp-tz)
[warning] 22-22: When Postgres stores a datetime in a timestamp field, Postgres drops the UTC offset. This means 2019-10-11 21:11:24+02 and 2019-10-11 21:11:24-06 will both be stored as 2019-10-11 21:11:24 in the database, even though they are eight hours apart in time. Use timestamptz instead of timestamp for your column type.
(prefer-timestamp-tz)
[warning] 23-23: When Postgres stores a datetime in a timestamp field, Postgres drops the UTC offset. This means 2019-10-11 21:11:24+02 and 2019-10-11 21:11:24-06 will both be stored as 2019-10-11 21:11:24 in the database, even though they are eight hours apart in time. Use timestamptz instead of timestamp for your column type.
(prefer-timestamp-tz)
[warning] 32-32: When Postgres stores a datetime in a timestamp field, Postgres drops the UTC offset. This means 2019-10-11 21:11:24+02 and 2019-10-11 21:11:24-06 will both be stored as 2019-10-11 21:11:24 in the database, even though they are eight hours apart in time. Use timestamptz instead of timestamp for your column type.
(prefer-timestamp-tz)
[warning] 33-33: When Postgres stores a datetime in a timestamp field, Postgres drops the UTC offset. This means 2019-10-11 21:11:24+02 and 2019-10-11 21:11:24-06 will both be stored as 2019-10-11 21:11:24 in the database, even though they are eight hours apart in time. Use timestamptz instead of timestamp for your column type.
(prefer-timestamp-tz)
[warning] 38-38: Using 32-bit integer fields can result in hitting the max int limit. Use 64-bit integer values instead to prevent hitting this limit.
(prefer-bigint-over-int)
[warning] 41-41: When Postgres stores a datetime in a timestamp field, Postgres drops the UTC offset. This means 2019-10-11 21:11:24+02 and 2019-10-11 21:11:24-06 will both be stored as 2019-10-11 21:11:24 in the database, even though they are eight hours apart in time. Use timestamptz instead of timestamp for your column type.
(prefer-timestamp-tz)
[warning] 42-42: When Postgres stores a datetime in a timestamp field, Postgres drops the UTC offset. This means 2019-10-11 21:11:24+02 and 2019-10-11 21:11:24-06 will both be stored as 2019-10-11 21:11:24 in the database, even though they are eight hours apart in time. Use timestamptz instead of timestamp for your column type.
(prefer-timestamp-tz)
[warning] 46-46: Using 32-bit integer fields can result in hitting the max int limit. Use 64-bit integer values instead to prevent hitting this limit.
(prefer-bigint-over-int)
[warning] 50-50: When Postgres stores a datetime in a timestamp field, Postgres drops the UTC offset. This means 2019-10-11 21:11:24+02 and 2019-10-11 21:11:24-06 will both be stored as 2019-10-11 21:11:24 in the database, even though they are eight hours apart in time. Use timestamptz instead of timestamp for your column type.
(prefer-timestamp-tz)
[warning] 51-51: Using 32-bit integer fields can result in hitting the max int limit. Use 64-bit integer values instead to prevent hitting this limit.
(prefer-bigint-over-int)
[warning] 65-65: When Postgres stores a datetime in a timestamp field, Postgres drops the UTC offset. This means 2019-10-11 21:11:24+02 and 2019-10-11 21:11:24-06 will both be stored as 2019-10-11 21:11:24 in the database, even though they are eight hours apart in time. Use timestamptz instead of timestamp for your column type.
(prefer-timestamp-tz)
[warning] 66-66: When Postgres stores a datetime in a timestamp field, Postgres drops the UTC offset. This means 2019-10-11 21:11:24+02 and 2019-10-11 21:11:24-06 will both be stored as 2019-10-11 21:11:24 in the database, even though they are eight hours apart in time. Use timestamptz instead of timestamp for your column type.
(prefer-timestamp-tz)
[warning] 72-72: wifi_role_speed_profile_speed_profile_id_wifi_speed_profile_id_fk is too long and will be truncated to 63 bytes.
(identifier-too-long)
🔇 Additional comments (36)
apps/chronos/src/utils/wifi/unifi-controller.ts (1)
1-142: LGTM!apps/chronos/src/utils/wifi/unifi-helpers.ts (1)
1-180: LGTM!apps/chronos/src/utils/wifi/controller.ts (1)
1-64: LGTM!apps/iris/src/components/admin/sidebar.tsx (1)
165-197: LGTM!Also applies to: 307-307
apps/iris/src/hooks/wifi-admin.ts (1)
18-102: LGTM!apps/iris/src/route-tree.gen.ts (1)
258-278: LGTM!apps/iris/src/routes/_private/route.tsx (1)
20-23: LGTM!apps/iris/src/routes/_private/admin/wifi/users.tsx (1)
162-182: 🗄️ Data Integrity & Integration
creatorNameis declared asz.string().nullable().optional()inwifiUserSchema, andWifiUseris inferred from that schema. OmittingcreatorNamefrom the synthetic orphan record does not cause the claimed type error.apps/iris/src/components/wifi/wifi-dialogs.tsx (1)
188-204: 🎯 Functional CorrectnessThe
itemsprop already renders the speed-profile options. The sharedSelectwrapper createsSelectContentand maps each item to aSelectItemwhenitemsis non-empty.WifiUserDialogtherefore does not need to renderSelectContentitself.apps/radius/.env.example (1)
1-8: LGTM!apps/radius/certs/.gitignore (1)
1-10: LGTM!apps/radius/certs/Makefile (1)
1-187: LGTM!apps/radius/freeradius/inner-tunnel (1)
1-68: LGTM!biome.jsonc (1)
210-211: LGTM!apps/chronos/.env.example (1)
109-132: LGTM!apps/chronos/src/database/migrations/0022_wifi_tables.sql (1)
1-83: LGTM!apps/chronos/src/database/migrations/0023_wide_midnight.sql (1)
1-2: LGTM!apps/chronos/src/database/migrations/meta/_journal.json (1)
158-171: LGTM!packages/api/src/domains/wifi/self.ts (1)
1-35: LGTM!Also applies to: 37-43, 45-63
packages/api/src/domains/wifi/stats.ts (1)
1-16: LGTM!packages/api/src/permissions.ts (1)
28-29: LGTM!apps/chronos/src/utils/cron.ts (1)
6-6: LGTM!Also applies to: 31-35
apps/chronos/src/utils/wifi/constants.ts (1)
1-3: LGTM!apps/chronos/src/utils/wifi/mac.ts (1)
1-18: LGTM!apps/chronos/src/database/index.ts (1)
15-15: LGTM!Also applies to: 54-54
apps/chronos/src/database/schema/wifi.ts (1)
1-147: LGTM!apps/chronos/src/database/scripts/reset.ts (1)
10-10: LGTM!Also applies to: 42-42
apps/chronos/src/utils/authorization.ts (1)
1-1: LGTM!Also applies to: 26-28, 141-145
apps/chronos/src/utils/wifi/cleanup.ts (1)
1-25: LGTM!apps/chronos/src/_types/hc.ts (1)
13-13: LGTM!Also applies to: 27-27
packages/api/src/domains/wifi/admin.ts (1)
1-176: LGTM!apps/chronos/src/index.ts (1)
30-30: LGTM!Also applies to: 105-105
apps/chronos/src/routes/wifi/_factory.ts (1)
1-4: LGTM!apps/chronos/src/routes/wifi/_router.ts (1)
37-80: LGTM!apps/chronos/src/routes/wifi/status.ts (1)
1-32: LGTM!apps/chronos/src/routes/wifi/radius.ts (1)
55-55: 🔒 Security & Privacy | 🛡️ Analyzed with Security ReviewEnsure the global rate limiter covers
/wifi/radius/authorize.authorizeRadiuswrites anwifiAuthLogrow before returning403for an invalid shared secret. If this route bypasses the global limiter, unauthenticated requests can create unbounded database writes. The limiter must run before this handler.
| const [inserted] = await db | ||
| .insert(wifiUser) | ||
| .values({ | ||
| allowedMacAddresses: allowedDevices, | ||
| banned: Boolean(user.banned), | ||
| comment: user.comment, | ||
| createdBy: userId || null, | ||
| encryptedPassword: user.password, | ||
| salt: user.salt, | ||
| userId: userId || null, | ||
| username: user.username, | ||
| }) | ||
| .returning({ id: wifiUser.id }); |
There was a problem hiding this comment.
🗄️ Data Integrity & Integration | 🟠 Major | 🏗️ Heavy lift
Make the migration atomic or idempotent.
A failure after this insert leaves migrated users in PostgreSQL. A retry then fails on the unique wifiUser.username constraint before it can resume device or NAS migration.
Run all destination writes in one transaction. Alternatively, add conflict handling and verify existing records before reuse.
Based on learnings, migration retries must account for partial state through a transaction, rollback, or idempotency.
Source: Learnings
| } | ||
|
|
||
| // biome-ignore lint/suspicious/noConsole: Migration script entrypoint | ||
| main().catch(console.error); |
There was a problem hiding this comment.
🩺 Stability & Availability | 🟠 Major | ⚡ Quick win
Return a non-zero exit status when the migration fails.
main().catch(console.error) handles the rejection. The process can then exit with status 0, so deployment tooling can report a failed or partial migration as successful.
Proposed fix
-main().catch(console.error);
+main().catch((error) => {
+ console.error(error);
+ process.exitCode = 1;
+});📝 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.
| main().catch(console.error); | |
| main().catch((error) => { | |
| console.error(error); | |
| process.exitCode = 1; | |
| }); |
| tags: ['WiFi'], | ||
| }), | ||
| ...authRouter(permissions.wifiRead), | ||
| zValidator('query', wifiListQuerySchema), |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟠 Major | ⚡ Quick win
Apply the wifiUserId device filter.
The API package defines wifiDeviceListQuerySchema, but this handler validates wifiListQuerySchema. Zod removes the unrecognized wifiUserId field. The handler then returns devices for all users.
Validate wifiDeviceListQuerySchema and add the corresponding wifiUserId condition.
| .from(wifiDevice) | ||
| .where(eq(wifiDevice.wifiUserId, account.id)); | ||
|
|
||
| return ok(c, { |
There was a problem hiding this comment.
🗄️ Data Integrity & Integration | 🟡 Minor | ⚡ Quick win
🔎 Supported by static analysis
🏁 Script executed:
sed -n '75,170p' apps/chronos/src/routes/wifi/self.ts
rg -n -C 8 'useCreateWifiAccount|createSelfWifiRoute|StatusCodes.CREATED|function ok|const ok' apps/iris/src/hooks/wifi.ts apps/chronos/src packagesRepository: filcdev/filc
Length of output: 14445
🏁 Script executed:
sed -n '1,95p' apps/iris/src/hooks/wifi.ts
rg -n -C 6 'parseResponse|response\.ok|status ===|201|Created|self\.\$post' apps/iris/src apps/chronos/src packages/apiRepository: filcdev/filc
Length of output: 50369
Return HTTP 201 for created WiFi accounts.
createSelfWifiRoute declares only 201, but ok(c, ...) defaults to 200. Iris checks response.ok, so account creation still succeeds. The response violates the OpenAPI contract and may affect clients that validate the declared status.
Proposed fix
- return ok(c, {
+ return ok(c, {
wifi: {
banned: account.banned,
devices,
speedLimit,
username: account.username,
},
- });
+ }, StatusCodes.CREATED);| const wifiUsername = userEmail.split('@')[0] || ''; | ||
| await db | ||
| .update(wifiUser) | ||
| .set({ createdBy: session.userId, userId: session.userId }) | ||
| .where( | ||
| and( | ||
| or( | ||
| eq(sql`lower(${wifiUser.username})`, wifiUsername), | ||
| eq(sql`lower(${wifiUser.username})`, userEmail) | ||
| ), | ||
| isNull(wifiUser.userId) | ||
| ) |
There was a problem hiding this comment.
🔒 Security & Privacy | 🛡️ Analyzed with Security Review | 🟠 Major | 🏗️ Heavy lift
IDOR
Reachability: External
Exploitability: Moderate
CWE: CWE-639 — Authorization Bypass Through User-Controlled Key (IDOR)
Do not use an email local part as a unique account identifier.
A user whose email local part matches an unlinked legacy WiFi username can claim that WiFi account during login. The ownership update then gives the user access to self-service operations, including password replacement.
apps/chronos/src/utils/authentication.ts#L97-L108: link by a verified full email or require one unambiguous migration mapping before assigninguserId.apps/chronos/scripts/migrate-petrikwifi.ts#L28-L38: detect duplicate local parts instead of overwriting the previous user inusernameToId.
📍 Affects 2 files
apps/chronos/src/utils/authentication.ts#L97-L108(this comment)apps/chronos/scripts/migrate-petrikwifi.ts#L28-L38
| } | ||
|
|
||
| # Points to the Chronos backend API | ||
| connect_uri = "http://chronos:3000/api/wifi" |
There was a problem hiding this comment.
🔒 Security & Privacy | 🛡️ Analyzed with Security Review | 🟠 Major | 🏗️ Heavy lift
Sensitive Data Exposure
Reachability: External
Exploitability: Moderate
CWE: CWE-319 — Cleartext Transmission of Sensitive Information
Encrypt and authenticate the Chronos REST connection.
This connection sends the FreeRADIUS shared secret to Chronos. Chronos returns the decrypted WiFi password in its response. Plain HTTP exposes both credentials to an attacker who can observe or alter traffic on the container network.
Use HTTPS. Enable certificate and hostname verification for every credential-bearing request.
| uri = "${..connect_uri}/radius/authorize" | ||
| method = 'post' | ||
| body = 'JSON' | ||
| data = '{"username": "%{User-Name}", "password": "%{User-Password}", "source": "%{Calling-Station-Id}", "destination": "%{Called-Station-Id}", "NAS": "%{NAS-Identifier}", "IP": "%{NAS-IP-Address}", "sharedSecret": "SECRET_HERE"}' |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟡 Minor | ⚡ Quick win
🔎 Supported by static analysis
🏁 Script executed:
#!/bin/bash
set -euo pipefail
entrypoint="$(fd -a '^docker-entrypoint\.sh$' apps/radius | head -n 1)"
compose="$(fd -a '^docker-compose\.yml$' apps/radius | head -n 1)"
printf '%s\n' '--- entrypoint substitution ---'
rg -n -C 5 'SECRET_HERE|FREERADIUS_SHARED_SECRET|sed|envsubst' "$entrypoint"
printf '%s\n' '--- environment propagation ---'
rg -n -C 5 'FREERADIUS_SHARED_SECRET|entrypoint|environment' "$compose"
printf '%s\n' '--- remaining placeholders ---'
rg -n 'SECRET_HERE' apps/radiusRepository: filcdev/filc
Length of output: 2650
🏁 Script executed:
#!/bin/bash
set -euo pipefail
printf '%s\n' '--- compose ---'
cat -n apps/radius/docker-compose.yml
printf '%s\n' '--- entrypoint context ---'
sed -n '1,70p' apps/radius/freeradius/docker-entrypoint.sh
printf '%s\n' '--- radius files ---'
git ls-files apps/radius | sed -n '1,120p'
printf '%s\n' '--- secret references ---'
rg -n -C 3 'FREERADIUS_SHARED_SECRET|freeradius.*secret|sharedSecret|docker-entrypoint' apps/radius README.md .github 2>/dev/null || trueRepository: filcdev/filc
Length of output: 6160
🏁 Script executed:
#!/bin/bash
set -euo pipefail
printf '%s\n' '--- REST configuration ---'
cat -n apps/radius/freeradius/rest | sed -n '1,40p'
printf '%s\n' '--- environment example ---'
cat -n apps/radius/.env.example
printf '%s\n' '--- configured endpoint and shared secret consumers ---'
rg -n -C 4 'radius/(authorize|authenticate)|sharedSecret|FREERADIUS_SHARED_SECRET|connect_uri' --glob '!apps/radius/freeradius/rest' --glob '!apps/radius/freeradius/docker-entrypoint.sh' .Repository: filcdev/filc
Length of output: 9376
Escape FREERADIUS_SHARED_SECRET before the sed replacement. The global replacement handles both SECRET_HERE values when the secret contains ordinary characters. However, & and backslashes change the generated value, while / makes sed fail. Because the entrypoint uses set -e, this can stop startup or cause Chronos to reject requests with an invalid sharedSecret. Use a replacement method that treats the secret literally.
| }); | ||
|
|
||
| export const wifiSelfPasswordChangeSchema = z.object({ | ||
| newPassword: z.string().min(1), |
There was a problem hiding this comment.
🔒 Security & Privacy | 🛡️ Analyzed with Security Review | 🟡 Minor | ⚡ Quick win
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
set -euo pipefail
rg -n -C 8 \
'wifiSelf(Create|PasswordChange)Schema|encryptPassword\(' \
apps/chronos/src packages/api/srcRepository: filcdev/filc
Length of output: 12991
🏁 Script executed:
#!/bin/bash
set -euo pipefail
printf '%s\n' '--- self schemas ---'
cat -n packages/api/src/domains/wifi/self.ts | sed -n '1,75p'
printf '%s\n' '--- self routes ---'
cat -n apps/chronos/src/routes/wifi/self.ts | sed -n '80,145p;260,320p'
printf '%s\n' '--- adjacent WiFi schemas and encryption binding ---'
rg -n -C 10 'export const wifi(User(Create|Update)?|.*Schema)|function encryptPassword|const encryptPassword|export .*encryptPassword' packages/api/src apps/chronos/srcRepository: filcdev/filc
Length of output: 29044
🤖 get_repo_knowledge executed:
get_repo_knowledge filcdev/filc /tmp/coderabbit-repo-knowledge/filcdev-filc-c4ce5d09/architecture
Length of output: 33536
Reachability: External
Exploitability: Moderate
CWE: CWE-521 — Weak Password Requirements
Enforce the WiFi password-strength policy.
Both schemas accept one-character passwords, and both self-service handlers pass the validated values directly to encryptPassword. Enforce the supported minimum and maximum password lengths in wifiSelfPasswordChangeSchema and wifiSelfCreateSchema.
| /> | ||
| <InputGroupAddon align="inline-end"> | ||
| <InputGroupButton | ||
| aria-label={showPassword ? 'Hide password' : 'Show password'} |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟡 Minor | ⚡ Quick win
Accept localized visibility labels.
This shared component fixes its accessible labels to English, although Iris provides common.showPassword and common.hidePassword translations. Accept both labels as props so each application can supply localized text.
Based on learnings, user-facing text in React components must use the application localization system.
🧰 Tools
🪛 Betterleaks (1.8.1)
[high] 31-31: Detected a potential hardcoded password literal, which may expose account credentials.
(generic-password)
Source: Learnings
| onClick={() => setShowPassword((prev) => !prev)} | ||
| onMouseDown={(e) => e.preventDefault()} | ||
| size="icon-xs" | ||
| tabIndex={-1} |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟡 Minor | ⚡ Quick win
Keep the visibility button in the keyboard tab order.
tabIndex={-1} prevents keyboard users from reaching the password visibility control. Remove this override so the button uses its normal tab position.
Proposed fix
- tabIndex={-1}📝 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.
| tabIndex={-1} |
| body: { | ||
| 'Cleartext-Password': password, | ||
| 'control:Cleartext-Password': password, |
There was a problem hiding this comment.
🔒 Security & Privacy | 🛡️ Analyzed with Security Review | 🟠 Major | 🏗️ Heavy lift
🧩 Analysis chain
🏁 Script executed:
sed -n '1,90p' apps/radius/freeradius/inner-tunnel
sed -n '1,80p' apps/radius/freeradius/rest
sed -n '75,145p' apps/chronos/src/utils/wifi/radius.ts
sed -n '260,330p' apps/chronos/src/utils/wifi/radius.ts
cat docs/freeradius-setup.mdRepository: filcdev/filc
Length of output: 8661
🏁 Script executed:
#!/bin/bash
set -eu
printf '%s\n' '--- FreeRADIUS virtual servers and EAP modules ---'
sed -n '1,220p' apps/radius/freeradius/default
sed -n '1,220p' apps/radius/freeradius/eap
sed -n '1,180p' apps/radius/freeradius/inner-eap
printf '%s\n' '--- Chronos radius route and API contract ---'
sed -n '1,130p' apps/chronos/src/routes/wifi/radius.ts
sed -n '1,180p' packages/api/src/domains/wifi/radius.ts
printf '%s\n' '--- Radius caller/request types and options ---'
rg -n -A35 -B12 'RadiusAuthorizeRequest|RadiusAuthorizeOptions|radiusAuthorizeResponseSchema|authorizeRadius|/radius/authorize|radius/ authenticate|radiusAuthenticate' apps/chronos/src packages/api/srcRepository: filcdev/filc
Length of output: 30398
🏁 Script executed:
#!/bin/bash
set -eu
printf '%s\n' '--- FreeRADIUS virtual servers and EAP modules ---'
sed -n '1,220p' apps/radius/freeradius/default
sed -n '1,220p' apps/radius/freeradius/eap
sed -n '1,180p' apps/radius/freeradius/inner-eap
printf '%s\n' '--- Chronos radius route and API contract ---'
sed -n '1,130p' apps/chronos/src/routes/wifi/radius.ts
sed -n '1,180p' packages/api/src/domains/wifi/radius.ts
printf '%s\n' '--- Radius caller/request types and options ---'
rg -n -A35 -B12 'RadiusAuthorizeRequest|RadiusAuthorizeOptions|radiusAuthorizeResponseSchema|authorizeRadius|/radius/authorize|radiusAuthenticate' apps/chronos/src packages/api/srcRepository: filcdev/filc
Length of output: 30398
Sensitive Data Exposure
Reachability: External
Exploitability: Difficult
CWE: CWE-200 — Exposure of Sensitive Information to an Unauthorized Actor
Avoid returning the reversible Wi-Fi password to FreeRADIUS.
The configured PEAP/MSCHAPv2 flow runs local mschap after REST authorization, so removing both password attributes would break authentication. However, the flow needs a password verifier, not specifically Cleartext-Password. Return a non-reversible verifier such as NT-Password, or reconfigure the inner authentication flow to delegate verification. Update the response schema and route accordingly.
The shared secret, source-IP, and NAS checks restrict this endpoint to the trusted FreeRADIUS client, but they do not remove the risk of exposing a reusable cleartext credential across that boundary.
# Conflicts: # apps/iris/src/route-tree.gen.ts
This branch has not been deployed
2 participants
Summary
🛑 DON'T BLINDLY MERGE, PLEASE TEST! 🛑
Please rigorously test the new database migrations locally.
Verify the frontend behavior when the WiFi module is toggled on/off.
Check admin dashboard and user self-service areas.
Double-check the FreeRADIUS REST and inner-EAP configurations.
Rollout Strategy & Migration Plan
Because this requires a migration from the old infrastructure on the WiFi side, I highly recommend keeping the WiFi integration flag set to false in production for now. (
CHRONOS_WIFI_ENABLED=false)We should schedule a maintenance window to perform the actual switch-over. During this window, not only do we need to migrate the users but also the FreeRADIUS keys from the old server so that clients are transferred seamlessly, avoiding any certificate validation issues on their end.
Verification
bun run lint(Agressively addedimport node:processto lots of things I didn't even touch? - so I set"noProcessGlobal": "off"- Probably should check this one, maybe useBun.where replacement is possible?)bun run typecheckbun run build— if bundler config, routers, or server entrypoints changedapps/iris/public/locales/en/translation.jsonandapps/iris/public/locales/hu/translation.json— if user-facing text changedbun run db:generateand committed — if the schema changedP.S: Thanks guys, keep up the good work 💖