fix: adversarial review findings for environment/raw endpoints#167
Merged
Conversation
Parse bedTemp/frzTemp CBOR records from RAW files into new biometrics DB tables, expose via tRPC with unit conversion. Add WiFi signal strength endpoint and RAW file list/download/delete API for disk management. - New DB tables: bed_temp (zone temps, humidity), freezer_temp (water temps) - Python module: environment-monitor (60s downsampled writes, 32MB/10% CPU) - Environment router: date-range queries, latest, summary with F/C conversion - System router: wifiStatus via nmcli with dev fallback - Raw router: file listing, safe delete (protects active file), disk usage - Next.js route: streaming binary download with path traversal protection - Deploy: install script updated for environment-monitor module
- nmcli parser: handle escaped colons in SSIDs - raw download: reject symlinks, verify canonical path - raw delete: mark as protected endpoint - Python module: INSERT OR IGNORE + seed cursors from DB on restart - Schema: unique indexes on timestamp (dedup guarantee) - Regenerate clean 0002 migration - CI build workflow: trigger on all branches, not just main - Service: run as dac user instead of root
- UNIQUE indexes on bed_temp.timestamp and freezer_temp.timestamp (migration SQL + drizzle schema) to prevent duplicate rows - Sync biometrics modules in sp-update flow so OTA updates install/restart piezo-processor, sleep-detector, environment-monitor - (Already in PR) symlink check + realpath validation in RAW route - (Already in PR) ENOENT vs 500 error differentiation - (Already in PR) INSERT OR IGNORE + DB-seeded cursors for restart safety - (Already in PR) deleteFile protect:true, nmcli escape-aware parsing - (Already in PR) User=dac in environment-monitor service Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
This reverts commit 6f928db.
- raw delete: canonical path + symlink checks (matches download route) - raw diskUsage: 5s timeout on df subprocess - wifiStatus: NaN guard on signal parsing - Python module: narrow except to sqlite3.Error, skip malformed records
- Delete orphaned migration 0002_mushy_loki.sql (not in journal) - Re-throw TRPCError in deleteFile catch block (was swallowed as 500) - Fix limit default(1440) > max(1000) schema bug → max(1440) - Parallelize getSummary queries with Promise.all - Add dev fallback for df -B1 in diskUsage (GNU-only flag) - Use SAFE_FILENAME filter in listRawFiles for consistency - Add 18 unit tests for tempUtils (centiDegrees, toF/toC, etc.) Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
|
Caution Review failedThe pull request is closed. ℹ️ Recent review info⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (1)
📝 WalkthroughWalkthroughPython backend introduces sensor data processing with centi-unit converters and database persistence for bed and freezer temperatures. TypeScript layer adds comprehensive unit tests for temperature conversions and new TRPC routers for environmental data retrieval and RAW file management with security controls. Changes
Estimated code review effort🎯 4 (Complex) | ⏱️ ~50 minutes Possibly related PRs
Poem
🚥 Pre-merge checks | ✅ 2 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches
🧪 Generate unit tests (beta)
📝 Coding Plan
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 |
Hardware emits bedTemp2 (not bedTemp) with nested left/right dicts containing floats in °C, and frzTemp with short field names (amb/hs/ left/right) as raw centidegrees. Convert bedTemp2 floats to centidegrees for consistent storage. Handle -327.68 sentinel as null.
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 4
🧹 Nitpick comments (4)
src/lib/tempUtils.ts (1)
39-39: Prefer composing throughcentiDegreesToCto avoid duplicated scale logic.♻️ Suggested refactor
-export const centiDegreesToF = (cd: number) => toF(cd / 100) +export const centiDegreesToF = (cd: number) => toF(centiDegreesToC(cd))🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/lib/tempUtils.ts` at line 39, The centiDegreesToF function duplicates scaling logic; change centiDegreesToF to reuse centiDegreesToC by converting cd -> centiDegreesToC(cd) and then passing that result to toF (i.e., centiDegreesToF should call centiDegreesToC(cd) and then toF on the Celsius result) so scaling logic lives only in centiDegreesToC and toF is used for the unit conversion.app/api/raw/[filename]/route.ts (1)
7-10: Extract shared constants to avoid drift.
RAW_DIRandSAFE_FILENAMEare duplicated between this file andsrc/server/routers/raw.ts. If either changes independently, security behavior could diverge.♻️ Suggested refactor
Extract these constants to a shared module (e.g.,
src/lib/rawConfig.ts):// src/lib/rawConfig.ts export const RAW_DIR = process.env.RAW_DATA_DIR ?? '/persistent' export const SAFE_FILENAME = /^[\w.-]+\.RAW$/iThen import in both files to ensure consistency.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@app/api/raw/`[filename]/route.ts around lines 7 - 10, RAW_DIR and SAFE_FILENAME are duplicated and may drift; extract them into a single shared module (e.g., export const RAW_DIR and export const SAFE_FILENAME from a new module) and replace the local declarations in both route handlers (the file containing RAW_DIR and SAFE_FILENAME and the server/router that also declares them) with imports from that module so both use the same canonical symbols RAW_DIR and SAFE_FILENAME.src/server/routers/environment.ts (1)
34-34: Consider defining typed output schemas.All procedures use
z.any()for output, which bypasses runtime validation and loses TypeScript inference for API consumers. While this simplifies initial development, typed schemas would improve API contracts and catch serialization issues.Also applies to: 74-74, 109-109, 146-146, 177-177
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/server/routers/environment.ts` at line 34, The procedures currently call .output(z.any()) which disables runtime validation and loses TypeScript inference; replace each .output(z.any()) in the environment router with a proper Zod schema (e.g., EnvironmentSchema, EnvironmentListSchema, CreateEnvironmentResponseSchema, UpdateEnvironmentResponseSchema) that matches the expected response shape, import or define those schemas near the top of src/server/routers/environment.ts, use them in the .output(...) calls that currently use z.any(), and export corresponding TypeScript types (z.infer<typeof ...>) so API consumers get correct typing and runtime validation for the procedures that currently contain .output(z.any()).src/db/biometrics-schema.ts (1)
53-78: LGTM! Schema correctly aligns with both the Python writer and tRPC API consumer.The table definitions match the column names used in
write_bed_tempandwrite_freezer_temp(Python), and the field selections ingetBedTemp,getFreezerTemp, andgetSummary(TypeScript). The unique indexes ontimestampproperly support theINSERT OR IGNOREdeduplication strategy.Minor naming inconsistency (optional): The unique indexes use an
idx_prefix (e.g.,idx_bed_temp_timestamp), while the existingvitalstable usesuq_for its unique index (uq_vitals_side_timestamp). Consider renaming touq_bed_temp_timestampanduq_freezer_temp_timestampfor consistency, though this is purely stylistic.,
♻️ Optional: Rename unique indexes for consistency
}, t => [ - uniqueIndex('idx_bed_temp_timestamp').on(t.timestamp), + uniqueIndex('uq_bed_temp_timestamp').on(t.timestamp), ]) export const freezerTemp = sqliteTable('freezer_temp', { ... }, t => [ - uniqueIndex('idx_freezer_temp_timestamp').on(t.timestamp), + uniqueIndex('uq_freezer_temp_timestamp').on(t.timestamp), ])🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/db/biometrics-schema.ts` around lines 53 - 78, Rename the unique index names to match the existing `vitals` convention by changing `idx_bed_temp_timestamp` to `uq_bed_temp_timestamp` and `idx_freezer_temp_timestamp` to `uq_freezer_temp_timestamp` in the table definitions for `bedTemp` and `freezerTemp` (the uniqueIndex calls that call .on(t.timestamp)); keep the same unique constraint logic and placement inside the index arrays so only the index identifier strings are updated while retaining the same .on(t.timestamp) targets.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@app/api/raw/`[filename]/route.ts:
- Around line 35-39: The startsWith check on canonicalFile vs canonicalDir can
be fooled by prefix matches (e.g., /persistent vs /persistent-backup); modify
the validation in the route handler that uses realpath, canonicalFile and
canonicalDir so you append a path separator to canonicalDir before calling
startsWith (use path.sep or ensure a trailing slash) and compare
canonicalFile.startsWith(canonicalDirWithSep) to guarantee the resolved file is
inside RAW_DIR; update the error branch that returns NextResponse.json({ error:
'Path traversal detected' }, { status: 400 }) to remain the same.
In `@modules/environment-monitor/main.py`:
- Around line 160-171: The DB connection and cursor seeding (open_biometrics_db,
last_bed_write, last_frz_write) are done before the guarded/protected startup
path so failures can abort the process without consistent health/fatal handling;
move that work into the existing protected startup/failure handling: wrap the
open_biometrics_db call, the two SELECTs that compute last_bed_write and
last_frz_write, and the RawFileFollower instantiation in a try/except block
inside the guarded startup sequence (the same area that uses _shutdown and
report_health), and on exception call report_health("unhealthy", "<contextual
error message>") and trigger the same shutdown/fatal handling used elsewhere
(invoke _shutdown or the existing exit path); ensure follower is only created
after DB seed succeeds and close/cleanup db_conn on failure.
In `@scripts/install`:
- Around line 680-708: The script currently swallows failures while syncing
biometrics modules; update the loop that processes MODULES_DEST and each module
(piezo-processor, sleep-detector, environment-monitor) so that failures creating
virtualenvs (python3 -m venv), installing requirements
("$MODULES_DEST/$mod/venv/bin/pip install ..."), copying the service file (cp
"$MODULES_DEST/$mod/$svc" ...), and systemctl enable/restart are checked for
non-zero exit status: on error, write a clear message to stderr (including $mod
and the failing command), set a failure flag (e.g. MODULE_SYNC_FAILED=1) and
continue to attempt other modules, and after the for loop exit non-zero (or
return a non-zero status) if MODULE_SYNC_FAILED is set so the installer does not
report success when module setup failed. Ensure messages reference the module
name and service variable svc to aid debugging.
In `@src/server/routers/raw.ts`:
- Around line 75-79: The startsWith check on canonicalFile vs canonicalDir in
raw.ts can produce false positives for directories with similar prefixes; update
the check that currently uses canonicalFile.startsWith(canonicalDir) to compare
canonicalFile against canonicalDir with a trailing path separator (e.g., ensure
canonicalDir ends with path.sep or '/' before calling startsWith) so only true
directory children pass; locate the code using realpath, canonicalFile,
canonicalDir and RAW_DIR and change the comparison to append the separator to
canonicalDir (or normalize both and compare with path utilities) to prevent
prefix-matching edge cases.
---
Nitpick comments:
In `@app/api/raw/`[filename]/route.ts:
- Around line 7-10: RAW_DIR and SAFE_FILENAME are duplicated and may drift;
extract them into a single shared module (e.g., export const RAW_DIR and export
const SAFE_FILENAME from a new module) and replace the local declarations in
both route handlers (the file containing RAW_DIR and SAFE_FILENAME and the
server/router that also declares them) with imports from that module so both use
the same canonical symbols RAW_DIR and SAFE_FILENAME.
In `@src/db/biometrics-schema.ts`:
- Around line 53-78: Rename the unique index names to match the existing
`vitals` convention by changing `idx_bed_temp_timestamp` to
`uq_bed_temp_timestamp` and `idx_freezer_temp_timestamp` to
`uq_freezer_temp_timestamp` in the table definitions for `bedTemp` and
`freezerTemp` (the uniqueIndex calls that call .on(t.timestamp)); keep the same
unique constraint logic and placement inside the index arrays so only the index
identifier strings are updated while retaining the same .on(t.timestamp)
targets.
In `@src/lib/tempUtils.ts`:
- Line 39: The centiDegreesToF function duplicates scaling logic; change
centiDegreesToF to reuse centiDegreesToC by converting cd -> centiDegreesToC(cd)
and then passing that result to toF (i.e., centiDegreesToF should call
centiDegreesToC(cd) and then toF on the Celsius result) so scaling logic lives
only in centiDegreesToC and toF is used for the unit conversion.
In `@src/server/routers/environment.ts`:
- Line 34: The procedures currently call .output(z.any()) which disables runtime
validation and loses TypeScript inference; replace each .output(z.any()) in the
environment router with a proper Zod schema (e.g., EnvironmentSchema,
EnvironmentListSchema, CreateEnvironmentResponseSchema,
UpdateEnvironmentResponseSchema) that matches the expected response shape,
import or define those schemas near the top of
src/server/routers/environment.ts, use them in the .output(...) calls that
currently use z.any(), and export corresponding TypeScript types (z.infer<typeof
...>) so API consumers get correct typing and runtime validation for the
procedures that currently contain .output(z.any()).
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 1aab7cf7-337f-4fdd-a7fd-0839d94ac917
📒 Files selected for processing (17)
app/api/raw/[filename]/route.tsmodules/environment-monitor/main.pymodules/environment-monitor/manifest.jsonmodules/environment-monitor/requirements.txtmodules/environment-monitor/sleepypod-environment-monitor.servicescripts/installsrc/db/biometrics-migrations/0002_fuzzy_strong_guy.sqlsrc/db/biometrics-migrations/meta/0002_snapshot.jsonsrc/db/biometrics-migrations/meta/_journal.jsonsrc/db/biometrics-schema.tssrc/lib/tempUtils.tssrc/lib/tests/tempUtils.test.tssrc/server/openapi.tssrc/server/routers/app.tssrc/server/routers/environment.tssrc/server/routers/raw.tssrc/server/routers/system.ts
Comment on lines
+35
to
+39
| const canonicalFile = await realpath(resolved) | ||
| const canonicalDir = await realpath(RAW_DIR) | ||
| if (!canonicalFile.startsWith(canonicalDir)) { | ||
| return NextResponse.json({ error: 'Path traversal detected' }, { status: 400 }) | ||
| } |
There was a problem hiding this comment.
Append path separator to prevent prefix-matching edge case.
If RAW_DIR is /persistent and a path resolves to /persistent-backup/file.RAW, the startsWith check would incorrectly pass. Append a path separator to ensure the file is within the directory, not just sharing a prefix.
🛡️ Proposed fix
const canonicalFile = await realpath(resolved)
const canonicalDir = await realpath(RAW_DIR)
- if (!canonicalFile.startsWith(canonicalDir)) {
+ if (!canonicalFile.startsWith(canonicalDir + path.sep)) {
return NextResponse.json({ error: 'Path traversal detected' }, { status: 400 })
}📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
Suggested change
| const canonicalFile = await realpath(resolved) | |
| const canonicalDir = await realpath(RAW_DIR) | |
| if (!canonicalFile.startsWith(canonicalDir)) { | |
| return NextResponse.json({ error: 'Path traversal detected' }, { status: 400 }) | |
| } | |
| const canonicalFile = await realpath(resolved) | |
| const canonicalDir = await realpath(RAW_DIR) | |
| if (!canonicalFile.startsWith(canonicalDir + path.sep)) { | |
| return NextResponse.json({ error: 'Path traversal detected' }, { status: 400 }) | |
| } |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@app/api/raw/`[filename]/route.ts around lines 35 - 39, The startsWith check
on canonicalFile vs canonicalDir can be fooled by prefix matches (e.g.,
/persistent vs /persistent-backup); modify the validation in the route handler
that uses realpath, canonicalFile and canonicalDir so you append a path
separator to canonicalDir before calling startsWith (use path.sep or ensure a
trailing slash) and compare canonicalFile.startsWith(canonicalDirWithSep) to
guarantee the resolved file is inside RAW_DIR; update the error branch that
returns NextResponse.json({ error: 'Path traversal detected' }, { status: 400 })
to remain the same.
Comment on lines
+160
to
+171
| db_conn = open_biometrics_db() | ||
| follower = RawFileFollower(RAW_DATA_DIR, _shutdown, poll_interval=0.5) | ||
|
|
||
| # Seed cursors from DB so restarts don't replay already-ingested samples | ||
| last_bed_write = float( | ||
| db_conn.execute("SELECT COALESCE(MAX(timestamp), 0) FROM bed_temp").fetchone()[0] or 0 | ||
| ) | ||
| last_frz_write = float( | ||
| db_conn.execute("SELECT COALESCE(MAX(timestamp), 0) FROM freezer_temp").fetchone()[0] or 0 | ||
| ) | ||
|
|
||
| report_health("healthy", "environment-monitor started") |
There was a problem hiding this comment.
Move startup DB initialization into a guarded failure path.
DB connection + cursor seeding currently happen before the protected loop; startup failures can terminate the process without consistent fatal handling/health status updates.
💡 Suggested fix
- db_conn = open_biometrics_db()
- follower = RawFileFollower(RAW_DATA_DIR, _shutdown, poll_interval=0.5)
-
- # Seed cursors from DB so restarts don't replay already-ingested samples
- last_bed_write = float(
- db_conn.execute("SELECT COALESCE(MAX(timestamp), 0) FROM bed_temp").fetchone()[0] or 0
- )
- last_frz_write = float(
- db_conn.execute("SELECT COALESCE(MAX(timestamp), 0) FROM freezer_temp").fetchone()[0] or 0
- )
-
- report_health("healthy", "environment-monitor started")
+ try:
+ db_conn = open_biometrics_db()
+ follower = RawFileFollower(RAW_DATA_DIR, _shutdown, poll_interval=0.5)
+
+ # Seed cursors from DB so restarts don't replay already-ingested samples
+ last_bed_write = float(
+ db_conn.execute("SELECT COALESCE(MAX(timestamp), 0) FROM bed_temp").fetchone()[0] or 0
+ )
+ last_frz_write = float(
+ db_conn.execute("SELECT COALESCE(MAX(timestamp), 0) FROM freezer_temp").fetchone()[0] or 0
+ )
+
+ report_health("healthy", "environment-monitor started")
+ except Exception as e:
+ log.exception("Startup failure: %s", e)
+ report_health("down", f"startup failure: {e}")
+ sys.exit(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.
Suggested change
| db_conn = open_biometrics_db() | |
| follower = RawFileFollower(RAW_DATA_DIR, _shutdown, poll_interval=0.5) | |
| # Seed cursors from DB so restarts don't replay already-ingested samples | |
| last_bed_write = float( | |
| db_conn.execute("SELECT COALESCE(MAX(timestamp), 0) FROM bed_temp").fetchone()[0] or 0 | |
| ) | |
| last_frz_write = float( | |
| db_conn.execute("SELECT COALESCE(MAX(timestamp), 0) FROM freezer_temp").fetchone()[0] or 0 | |
| ) | |
| report_health("healthy", "environment-monitor started") | |
| try: | |
| db_conn = open_biometrics_db() | |
| follower = RawFileFollower(RAW_DATA_DIR, _shutdown, poll_interval=0.5) | |
| # Seed cursors from DB so restarts don't replay already-ingested samples | |
| last_bed_write = float( | |
| db_conn.execute("SELECT COALESCE(MAX(timestamp), 0) FROM bed_temp").fetchone()[0] or 0 | |
| ) | |
| last_frz_write = float( | |
| db_conn.execute("SELECT COALESCE(MAX(timestamp), 0) FROM freezer_temp").fetchone()[0] or 0 | |
| ) | |
| report_health("healthy", "environment-monitor started") | |
| except Exception as e: | |
| log.exception("Startup failure: %s", e) | |
| report_health("down", f"startup failure: {e}") | |
| sys.exit(1) |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@modules/environment-monitor/main.py` around lines 160 - 171, The DB
connection and cursor seeding (open_biometrics_db, last_bed_write,
last_frz_write) are done before the guarded/protected startup path so failures
can abort the process without consistent health/fatal handling; move that work
into the existing protected startup/failure handling: wrap the
open_biometrics_db call, the two SELECTs that compute last_bed_write and
last_frz_write, and the RawFileFollower instantiation in a try/except block
inside the guarded startup sequence (the same area that uses _shutdown and
report_health), and on exception call report_health("unhealthy", "<contextual
error message>") and trigger the same shutdown/fatal handling used elsewhere
(invoke _shutdown or the existing exit path); ensure follower is only created
after DB seed succeeds and close/cleanup db_conn on failure.
Comment on lines
+680
to
+708
| # Sync biometrics modules | ||
| if [ -d "$INSTALL_DIR/modules" ]; then | ||
| MODULES_DEST="/opt/sleepypod/modules" | ||
| mkdir -p "$MODULES_DEST" | ||
| if [ -d "$INSTALL_DIR/modules/common" ]; then | ||
| mkdir -p "$MODULES_DEST/common" | ||
| cp -r "$INSTALL_DIR/modules/common/." "$MODULES_DEST/common/" | ||
| fi | ||
| for mod in piezo-processor sleep-detector environment-monitor; do | ||
| if [ -d "$INSTALL_DIR/modules/$mod" ]; then | ||
| mkdir -p "$MODULES_DEST/$mod" | ||
| cp -r "$INSTALL_DIR/modules/$mod/." "$MODULES_DEST/$mod/" | ||
| if [ ! -d "$MODULES_DEST/$mod/venv" ] && command -v python3 &>/dev/null; then | ||
| python3 -m venv "$MODULES_DEST/$mod/venv" 2>/dev/null || true | ||
| fi | ||
| if [ -d "$MODULES_DEST/$mod/venv" ] && [ -f "$MODULES_DEST/$mod/requirements.txt" ]; then | ||
| "$MODULES_DEST/$mod/venv/bin/pip" install --quiet -r "$MODULES_DEST/$mod/requirements.txt" 2>/dev/null || true | ||
| fi | ||
| svc="sleepypod-$mod.service" | ||
| if [ -f "$MODULES_DEST/$mod/$svc" ]; then | ||
| cp "$MODULES_DEST/$mod/$svc" "/etc/systemd/system/$svc" | ||
| systemctl daemon-reload | ||
| systemctl enable "$svc" 2>/dev/null || true | ||
| systemctl restart "$svc" 2>/dev/null || true | ||
| fi | ||
| fi | ||
| done | ||
| fi | ||
|
|
There was a problem hiding this comment.
Don’t silently ignore biometrics module sync failures.
Dependency install/service enable-restart failures are currently swallowed, so updates can report success while modules are non-functional.
💡 Suggested fix
- for mod in piezo-processor sleep-detector environment-monitor; do
+ module_sync_failed=false
+ for mod in piezo-processor sleep-detector environment-monitor; do
if [ -d "$INSTALL_DIR/modules/$mod" ]; then
@@
- if [ -d "$MODULES_DEST/$mod/venv" ] && [ -f "$MODULES_DEST/$mod/requirements.txt" ]; then
- "$MODULES_DEST/$mod/venv/bin/pip" install --quiet -r "$MODULES_DEST/$mod/requirements.txt" 2>/dev/null || true
+ if [ -d "$MODULES_DEST/$mod/venv" ] && [ -f "$MODULES_DEST/$mod/requirements.txt" ]; then
+ if ! "$MODULES_DEST/$mod/venv/bin/pip" install --quiet -r "$MODULES_DEST/$mod/requirements.txt"; then
+ echo "Warning: failed to install requirements for $mod" >&2
+ module_sync_failed=true
+ fi
fi
@@
- systemctl enable "$svc" 2>/dev/null || true
- systemctl restart "$svc" 2>/dev/null || true
+ if ! systemctl enable "$svc"; then
+ echo "Warning: failed to enable $svc" >&2
+ module_sync_failed=true
+ fi
+ if ! systemctl restart "$svc"; then
+ echo "Warning: failed to restart $svc" >&2
+ module_sync_failed=true
+ fi
fi
fi
done
+ if [ "$module_sync_failed" = true ]; then
+ echo "Warning: one or more biometrics modules failed to sync; check module service logs." >&2
+ fi
fi🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@scripts/install` around lines 680 - 708, The script currently swallows
failures while syncing biometrics modules; update the loop that processes
MODULES_DEST and each module (piezo-processor, sleep-detector,
environment-monitor) so that failures creating virtualenvs (python3 -m venv),
installing requirements ("$MODULES_DEST/$mod/venv/bin/pip install ..."), copying
the service file (cp "$MODULES_DEST/$mod/$svc" ...), and systemctl
enable/restart are checked for non-zero exit status: on error, write a clear
message to stderr (including $mod and the failing command), set a failure flag
(e.g. MODULE_SYNC_FAILED=1) and continue to attempt other modules, and after the
for loop exit non-zero (or return a non-zero status) if MODULE_SYNC_FAILED is
set so the installer does not report success when module setup failed. Ensure
messages reference the module name and service variable svc to aid debugging.
Comment on lines
+75
to
+79
| const canonicalFile = await realpath(resolved) | ||
| const canonicalDir = await realpath(RAW_DIR) | ||
| if (!canonicalFile.startsWith(canonicalDir)) { | ||
| throw new TRPCError({ code: 'BAD_REQUEST', message: 'Path traversal detected' }) | ||
| } |
There was a problem hiding this comment.
Same path separator edge case as route.ts.
The startsWith check on canonical paths should append a path separator to prevent prefix-matching edge cases (e.g., /persistent vs /persistent-backup).
🛡️ Proposed fix
const canonicalFile = await realpath(resolved)
const canonicalDir = await realpath(RAW_DIR)
- if (!canonicalFile.startsWith(canonicalDir)) {
+ if (!canonicalFile.startsWith(canonicalDir + path.sep)) {
throw new TRPCError({ code: 'BAD_REQUEST', message: 'Path traversal detected' })
}🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@src/server/routers/raw.ts` around lines 75 - 79, The startsWith check on
canonicalFile vs canonicalDir in raw.ts can produce false positives for
directories with similar prefixes; update the check that currently uses
canonicalFile.startsWith(canonicalDir) to compare canonicalFile against
canonicalDir with a trailing path separator (e.g., ensure canonicalDir ends with
path.sep or '/' before calling startsWith) so only true directory children pass;
locate the code using realpath, canonicalFile, canonicalDir and RAW_DIR and
change the comparison to append the separator to canonicalDir (or normalize both
and compare with path utilities) to prevent prefix-matching edge cases.
…anagement # Conflicts: # modules/environment-monitor/main.py # src/server/routers/environment.ts # src/server/routers/raw.ts
|
🎉 This PR is included in version 1.1.0 🎉 The release is available on GitHub release Your semantic-release bot 📦🚀 |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
Follow-up to #162 — addresses findings from dual-model adversarial code review.
0002_mushy_loki.sql(not in drizzle journal)TRPCErrorindeleteFilecatch block (was swallowed as 500 instead of 400)limitschema: default(1440) exceeded max(1000) — raised max to 1440getSummaryqueries withPromise.alldf -B1indiskUsage(GNU-only, fails on macOS)SAFE_FILENAMEfilter inlistRawFilesfor consistency with download/deletetempUtils(centiDegrees conversions, toF/toC, ensureF, mapToEightSleepScale)Test plan
🤖 Generated with Claude Code
Summary by CodeRabbit
Release Notes
New Features
Tests