feat(companion): record scan location on native QR scans - #2768
feat(companion): record scan location on native QR scans#2768carlosvirreira wants to merge 1 commit into
Conversation
Web records where a QR was scanned (updateScanGeolocation on the public qr route); the companion recorded who and when but never where, so an asset's last-known-location was blank for every mobile scan. This closes that parity gap. - mobile QR resolve accepts optional x-shelf-scan-latitude / x-shelf-scan-longitude HEADERS (Zod-validated, range-checked); invalid or partial pairs are ignored, never an error — provenance must not be able to fail a resolve - headers rather than query params so precise coordinates never enter access logs, APM traces or Sentry breadcrumbs; scan service errors no longer carry coordinates in additionalData - companion sends a cached/last-known fix raced against a 1500ms timeout, refreshing in the background; a scan is NEVER delayed or blocked on location, and a denied permission simply records without coordinates - one lazy permission request, only from the scanner and only once camera is granted, so the prompts never stack on first run - 5-minute staleness cap: an hour-old fix is wrong provenance, not degraded provenance - privacy manifest declares precise location (linked, app functionality) Tested: 12 route tests incl. malformed/partial/out-of-range and an empty-string 'Null Island' regression; verified end-to-end on an iPhone against a local server — a real scan wrote 51.9795/5.9813 to Scan.
🩺 React Doctor — companionNo diagnostics directory passed — scan may have failed (CLI exit unknown). Check the workflow logs. |
🩺 React Doctor — webappNo diagnostics directory passed — scan may have failed (CLI exit unknown). Check the workflow logs. |
![hex-security-app[bot]](https://avatars.githubusercontent.com/in/2665233?s=60&v=4){kind=small}
| additionalData: { id }, | ||
| label, | ||
| }); | ||
| } |
There was a problem hiding this comment.
Originally flagged line 268; mapped to the nearest line in the diff (136).
The PR deliberately strips latitude/longitude from createScan and updateScan error additionalData to prevent GPS leaking into pino logs and Sentry. However, createScanNote — called internally by createScan with the same coordinates — still includes both fields in its catch-block additionalData (line 268). The logger's serializeError function recursively walks error.cause chains, so when createScanNote throws, the nested ShelfError (with GPS in additionalData) is fully serialized into the outer error. Any createScanNote failure (DB error, user lookup failure, etc.) will emit the user's precise GPS to pino and Sentry, exactly the outcome the rest of the PR guards against.
Prompt To Fix With AI
In the `createScanNote` catch block (line ~264-272 of service.server.ts), remove `latitude`, `longitude`, and `manuallyGenerated` from `additionalData` to match the same GPS-scrubbing policy applied to `createScan` and `updateScan`. Only keep identifiers needed to trace the failure: `userId` and `qrId`. Change: `additionalData: { userId, qrId, latitude, longitude, manuallyGenerated }` → `additionalData: { userId, qrId }`.Severity: medium | Confidence: 85% | React with 👍 if useful or 👎 if not
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: e7fd8bef71
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| "expo-image-manipulator": "~14.0.8", | ||
| "expo-image-picker": "~17.0.10", | ||
| "expo-linking": "~8.0.11", | ||
| "expo-location": "~19.0.8", |
There was a problem hiding this comment.
Adding this dependency without updating pnpm-lock.yaml makes every frozen workspace install fail with ERR_PNPM_OUTDATED_LOCKFILE before builds or tests can run. This affects the checked CI workflows in .github/workflows/test.yml, .github/workflows/react-doctor.yml, and .github/workflows/docs-deploy.yml, as well as the webapp Docker builds, all of which use pnpm install --frozen-lockfile; regenerate and commit the lockfile with pnpm.
AGENTS.md reference: AGENTS.md:L3-L3
Useful? React with 👍 / 👎.
| // `additionalData` is forwarded to the logger and to Sentry. Precise | ||
| // user location must never enter the error pipeline; the QR id is what | ||
| // makes a failing scan traceable. | ||
| additionalData: { qrId }, |
There was a problem hiding this comment.
When createScanNote fails after a geolocated mobile scan, this outer redaction is ineffective: createScanNote constructs its own ShelfError with additionalData: { userId, qrId, latitude, longitude, manuallyGenerated } at lines 264–269, that error remains in this error's cause chain, and Logger.error recursively serializes causes before sending the route failure to logs and Sentry. Thus a transient note or database failure leaks the user's precise GPS despite the stated redaction; remove the coordinates from the nested error as well.
Useful? React with 👍 / 👎.
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Organization UI Review profile: CHILL Plan: Pro Plus Run ID: 📒 Files selected for processing (11)
WalkthroughThe companion app adds cached geolocation acquisition and sends scan coordinates through QR requests. The mobile QR endpoint validates coordinate headers and forwards valid values to scan creation. Platform privacy declarations and scan error metadata are also updated. ChangesScan geolocation provenance
Scan error context
Estimated code review effort: 4 (Complex) | ~45 minutes Sequence Diagram(s)sequenceDiagram
participant Scanner
participant ScanLocation
participant CompanionQRClient
participant MobileQRRoute
participant CreateScan
Scanner->>ScanLocation: Retrieve scan coordinates
ScanLocation-->>Scanner: Coordinates or null
Scanner->>CompanionQRClient: Resolve QR with coordinates
CompanionQRClient->>MobileQRRoute: Send latitude/longitude headers
MobileQRRoute->>CreateScan: Pass validated coordinates
Possibly related PRs
Suggested reviewers: 🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✨ Finishing Touches 💡 2📝 Generate docstrings 💡
🛠️ Fix failing CI checks 💡
🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
1 participant
Why this exists
Web has recorded scan geolocation since forever (
updateScanGeolocationon the public/qr/:idroute). The companion recorded who and when but never where — so for every scan made with our own app, "last known location" was blank. Carlos flagged the gap while dogfooding; the mobile resolver's JSDoc had it as an acknowledged deferral ("GPS coordinates are intentionally NOT captured here — a separate, deliberate item"). This is that item.Intent
Provenance should never be able to hurt the thing it's documenting. Two rules drove every decision:
createScanfailures no longer carry them inadditionalDataeither.The transport point is worth flagging: the first implementation used
?latitude=&longitude=. Our own security review caught that it diverged from the working web sibling (which POSTs coordinates in a body) and would have written customer GPS into every request log. Changed before this PR existed.Behaviour
Test plan
z.coerce.number()turns""into0— coordinates off West Africa; we reject instead).51.97957 / 5.98138to theScanrow, via headers, with the app reporting no perceptible delay.validategreen; companiontsc+expo lintclean.Rollout notes for review
expo-locationpod) — not OTA-able. Sequenced ahead of the quantity-trust PR so it can ride the next binary either way.NSPrivacyCollectedDataTypePreciseLocation(linked, app-functionality, non-tracking). The privacy policy should mention scan location too — flagging as a release task, not a code one.🤖 Generated with Claude Code
Summary by CodeRabbit
New Features
Bug Fixes
Documentation