Add Bluetooth device scanning and cloning for security testing#9
Conversation
Co-authored-by: dshvvvshr <216969053+dshvvvshr@users.noreply.github.com>
Co-authored-by: dshvvvshr <216969053+dshvvvshr@users.noreply.github.com>
Co-authored-by: dshvvvshr <216969053+dshvvvshr@users.noreply.github.com>
Co-authored-by: dshvvvshr <216969053+dshvvvshr@users.noreply.github.com>
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 0ed8637d63
ℹ️ 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".
src/security/bluetooth.ts
Outdated
| if (noble._state === 'poweredOn') { | ||
| resolve(true); | ||
| } else { | ||
| noble.once('stateChange', (state) => { | ||
| resolve(state === 'poweredOn'); |
There was a problem hiding this comment.
Resolve isReady when state is already non-poweredOn
If noble._state is already a terminal non-powered state (e.g., unsupported, unauthorized) and the initial stateChange event has already fired before isReady() is called, the promise never resolves because it only listens for a future stateChange. In that situation, startScan()/scan() await forever instead of cleanly returning false or throwing. Consider resolving immediately for any known non-poweredOn state (or adding a timeout) so callers don’t hang on systems without usable Bluetooth.
Useful? React with 👍 / 👎.
| this.discoveredDevices.clear(); | ||
| this.scanning = true; | ||
|
|
||
| auditLogger.log(AuditLevel.INFO, 'bluetooth', 'scan-start', { | ||
| options, |
There was a problem hiding this comment.
Reset scanning flag if startScanningAsync fails
this.scanning is set to true before the call to startScanningAsync(). If startScanningAsync() rejects (permissions, adapter busy, or driver errors), the flag remains true, and subsequent calls will throw “Scan already in progress” even though no scan is running. Wrapping the start in try/catch and resetting this.scanning on failure (or setting it after a successful start) avoids the false “busy” state.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Pull request overview
Adds a new Bluetooth security module to Prime Security, enabling BLE device discovery and basic “profile cloning” for offline analysis.
Changes:
- Introduces
BluetoothScannerfor BLE scanning, device identification, clone creation, and JSON import/export. - Registers a new
bluetooth-securitymodule in the system registry and exposes it from the package entrypoint. - Adds an example script, basic Jest tests, README updates, and noble dependencies.
Reviewed changes
Copilot reviewed 6 out of 6 changed files in this pull request and generated 10 comments.
Show a summary per file
| File | Description |
|---|---|
src/security/bluetooth.ts |
Implements BLE scanning, device caching, clone creation, and JSON import/export APIs. |
src/index.ts |
Registers bluetooth-security module and re-exports the bluetooth module API. |
src/examples/bluetooth-scanner.ts |
Demonstrates scanning, cloning, and exporting a device profile. |
tests/security/bluetooth.test.ts |
Adds initial tests for data structures (non-hardware). |
package.json |
Adds @abandonware/noble runtime dependency and @types/noble dev dependency. |
README.md |
Updates documentation to describe Bluetooth scanning/cloning and project structure. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
src/security/bluetooth.ts
Outdated
| return new Promise((resolve) => { | ||
| setTimeout(async () => { | ||
| await this.stopScan(); | ||
| const scanDuration = Date.now() - startTime; | ||
|
|
||
| resolve({ | ||
| devices: Array.from(this.discoveredDevices.values()), | ||
| scanDuration, | ||
| timestamp: new Date(), | ||
| }); |
There was a problem hiding this comment.
The async setTimeout callback doesn’t handle errors. If stopScan() throws, the callback rejects and resolve(...) is never called, leaving the scan() promise pending (and causing an unhandled rejection). Wrap the body in try/catch and resolve/reject deterministically.
| return new Promise((resolve) => { | |
| setTimeout(async () => { | |
| await this.stopScan(); | |
| const scanDuration = Date.now() - startTime; | |
| resolve({ | |
| devices: Array.from(this.discoveredDevices.values()), | |
| scanDuration, | |
| timestamp: new Date(), | |
| }); | |
| return new Promise((resolve, reject) => { | |
| setTimeout(async () => { | |
| try { | |
| await this.stopScan(); | |
| const scanDuration = Date.now() - startTime; | |
| resolve({ | |
| devices: Array.from(this.discoveredDevices.values()), | |
| scanDuration, | |
| timestamp: new Date(), | |
| }); | |
| } catch (error) { | |
| reject(error); | |
| } |
src/security/bluetooth.ts
Outdated
| if (!this.scanning) { | ||
| return; | ||
| } | ||
|
|
||
| await noble.stopScanningAsync(); | ||
| this.scanning = false; | ||
|
|
||
| auditLogger.log(AuditLevel.INFO, 'bluetooth', 'scan-stop', { | ||
| devicesFound: this.discoveredDevices.size, | ||
| }); |
There was a problem hiding this comment.
stopScan() is not safe against concurrent calls: this.scanning remains true until after await noble.stopScanningAsync(), so two callers can both pass the guard and call stopScanningAsync() twice. Add a stopping flag or store an in-flight promise so only one stop operation runs at a time.
| if (!this.scanning) { | |
| return; | |
| } | |
| await noble.stopScanningAsync(); | |
| this.scanning = false; | |
| auditLogger.log(AuditLevel.INFO, 'bluetooth', 'scan-stop', { | |
| devicesFound: this.discoveredDevices.size, | |
| }); | |
| const self = this as any; | |
| // If a stop operation is already in progress, wait for it instead of starting another. | |
| if (self._stopScanPromise) { | |
| await self._stopScanPromise; | |
| return; | |
| } | |
| if (!this.scanning) { | |
| return; | |
| } | |
| self._stopScanPromise = (async () => { | |
| try { | |
| await noble.stopScanningAsync(); | |
| this.scanning = false; | |
| auditLogger.log(AuditLevel.INFO, 'bluetooth', 'scan-stop', { | |
| devicesFound: this.discoveredDevices.size, | |
| }); | |
| } finally { | |
| self._stopScanPromise = null; | |
| } | |
| })(); | |
| await self._stopScanPromise; |
src/security/bluetooth.ts
Outdated
| importClone(cloneJson: string): DeviceClone { | ||
| let clone: DeviceClone; | ||
|
|
||
| try { | ||
| clone = JSON.parse(cloneJson) as DeviceClone; |
There was a problem hiding this comment.
importClone() parses JSON and casts to DeviceClone, but metadata.clonedAt will be a string after JSON.parse, not a Date. Either store timestamps as ISO strings in the public clone format, or normalize (clone.metadata.clonedAt = new Date(...)) before returning/storing. Right now the runtime value won’t match the declared type.
| return { | ||
| scanning: this.scanning, | ||
| bluetoothState: noble._state, // Public API despite underscore prefix | ||
| discoveredCount: this.discoveredDevices.size, |
There was a problem hiding this comment.
getStatus() (and isReady()) rely on noble._state. The underscore-prefixed property is an internal implementation detail in most libraries and can change without notice. Prefer a documented/public property (if available) or track the last state from the stateChange event in the scanner itself.
| it('should allow creating and manipulating device clone data', () => { | ||
| // Test clone data structure without requiring hardware | ||
| const cloneData = { | ||
| deviceId: 'test-device-123', |
There was a problem hiding this comment.
These tests only assert behavior of ad-hoc plain objects and never import/exercise BluetoothScanner (e.g., exportClone()/importClone() and import validation), so they won’t catch regressions in the actual module implementation. Add non-hardware tests that instantiate the scanner and cover JSON export/import (including invalid JSON / missing fields).
README.md
Outdated
| 2. **Identify** devices by their address, name, services, and characteristics | ||
| 3. **Clone** device profiles including: | ||
| - Device address and name | ||
| - Advertisement data (manufacturer data, service UUIDs, tx power) | ||
| - Service and characteristic information |
There was a problem hiding this comment.
The README says device identification/cloning includes "services and characteristics" / "Service and characteristic information", but the current implementation only captures advertisement fields and sets profile.services to an empty array. Please align the documentation with what the code actually provides, or implement service/characteristic discovery and populate the clone profile accordingly.
src/security/bluetooth.ts
Outdated
|
|
||
| await this.startScan({ ...options, duration }); |
There was a problem hiding this comment.
scan() calls startScan({ ..., duration }), and startScan() already schedules an automatic stopScan() when options.duration is set. scan() then schedules its own timeout as well, which can lead to multiple stopScan() calls racing and potentially invoking noble.stopScanningAsync() twice. Consider having only one place responsible for the stop timer (either scan() or startScan()), or make stopping explicitly idempotent with a shared in-flight stop promise.
| await this.startScan({ ...options, duration }); | |
| const { duration: _duration, ...scanOptions } = options; | |
| await this.startScan(scanOptions); |
src/security/bluetooth.ts
Outdated
| auditLogger.log(AuditLevel.INFO, 'bluetooth', 'device-cloned', { | ||
| deviceId: device.id, | ||
| name: device.name, | ||
| cloneId: hash(device.id + Date.now().toString()).substring(0, 16), |
There was a problem hiding this comment.
A cloneId is generated for logging, but clones are stored in clonedDevices keyed by device.id and the returned clone uses deviceId: device.id. This makes the logged cloneId unusable and prevents keeping multiple clones of the same device. Either remove cloneId from the log, or include/persist it (and key the map by it) if multiple clones are intended.
| cloneId: hash(device.id + Date.now().toString()).substring(0, 16), |
| return _bluetoothScanner; | ||
| }, | ||
| reset(): void { | ||
| _bluetoothScanner = null; | ||
| } |
There was a problem hiding this comment.
bluetoothScanner.reset() drops the singleton instance but does not unregister the noble.on(...) listeners that were added in the constructor. Creating a new instance after reset will stack additional listeners (duplicate logs/handlers and potential memory leak). Consider adding a destroy()/dispose() that removes listeners (or avoid exposing reset() in production code).
README.md
Outdated
| For issues and questions: | ||
| - Open an issue on GitHub | ||
| - Check the Brave Search API documentation: https://brave.com/search/api/ | ||
| - Review the documentation in the `/docs` directory |
There was a problem hiding this comment.
README references "the /docs directory", but there is no docs/ directory in the repository. Either add the documentation directory or update this pointer to the actual location of docs.
| - Review the documentation in the `/docs` directory | |
| - Review the documentation in this repository |
|
@copilot apply changes based on the comments in this thread |
… docs, and tests Co-authored-by: dshvvvshr <216969053+dshvvvshr@users.noreply.github.com>
All review comments have been addressed in commit 3735dac: Concurrency & Error Handling:
State Management:
Data Validation:
Testing:
Documentation:
Tests are passing! ✅ All 27 tests pass successfully:
Build is clean with no TypeScript errors. All review feedback has been addressed in commit 3735dac. |
Implementation Complete: Bluetooth Scanning and Device Cloning ✅
All review comments have been addressed!
Changes Made
Core Fixes
stopScan()now uses promise tracking to prevent race conditionsbluetoothStatefield tracks state from events, no longer usesnoble._stateremoveNobleListeners()and proper cleanup inreset()scan()timeout properly handles errors with try/catch and rejectimportClone()converts JSON date strings to Date objectsunknownand proper type guardsDocumentation
/docsdirectoryTesting
Final Status
✅ Build successful
✅ All tests passing (27/27)
✅ Linting clean (core code)
✅ All review comments addressed
✅ Security scan passed
Original prompt
✨ Let Copilot coding agent set things up for you — coding agent works faster and does higher quality work when set up for your repo.