Conversation
…PIN undeclared error Co-authored-by: dorkmo <1923070+dorkmo@users.noreply.github.com>
There was a problem hiding this comment.
Pull request overview
Updates session token generation in the Server (Blues Opta) sketch to avoid referencing VIN_ANALOG_PIN before it is defined later in the .ino file.
Changes:
- Adjusts
generateSessionToken()to readA0directly (instead ofVIN_ANALOG_PIN) when seeding the session token RNG. - Adds clarifying comments explaining the compile-order motivation.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
You can also share your feedback on Copilot code review. Take the survey.
| // Mix micros(), millis(), and ADC noise for entropy. | ||
| // Use A0 directly here; VIN_ANALOG_PIN is defined later in the file. | ||
| uint32_t seed = micros() ^ (millis() << 16) ^ ((uint32_t)analogRead(A0) << 8); |
There was a problem hiding this comment.
generateSessionToken() now hardcodes analogRead(A0). That avoids the compile-order issue, but it also ignores VIN_ANALOG_PIN overrides (e.g., if someone configures the VIN divider on A1, the session token still samples A0). Prefer defining VIN_ANALOG_PIN (or a dedicated ENTROPY_ANALOG_PIN) before this function and using that symbol here, so the behavior stays consistent with configuration and doesn’t assume A0 is available/free.
| // Mix micros(), millis(), and ADC noise for entropy. | ||
| // Use A0 directly here; VIN_ANALOG_PIN is defined later in the file. | ||
| uint32_t seed = micros() ^ (millis() << 16) ^ ((uint32_t)analogRead(A0) << 8); | ||
| for (int i = 0; i < 16; i++) { | ||
| seed = seed * 1103515245 + 12345; | ||
| gSessionToken[i] = "0123456789abcdef"[(seed >> 16) & 0xF]; |
There was a problem hiding this comment.
Session tokens are used as an authentication/session mechanism, but the generator is a simple LCG seeded with micros()/millis() and a single analogRead, which is not a cryptographically secure RNG and can be guessable if an attacker can estimate boot/login timing and the analog pin is driven. Consider sourcing randomness from a hardware-backed/secure source (or at least mixing in additional unpredictable state and using a stronger construction) to reduce token predictability.
|
@copilot apply changes based on the comments in this thread |
…y pins, better mixing Co-authored-by: dorkmo <1923070+dorkmo@users.noreply.github.com>
Done in commit
|
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 1 out of 1 changed files in this pull request and generated 2 comments.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
You can also share your feedback on Copilot code review. Take the survey.
| uint32_t seed = t0 ^ (currentMillis << 13) ^ (currentMillis >> 19) | ||
| ^ (a0 << 5) ^ (a0 >> 27) | ||
| ^ (a1 << 11) ^ (a2 << 21) | ||
| ^ (a3 << 7) ^ (t1 << 17); | ||
| for (int i = 0; i < 16; i++) { | ||
| seed = seed * 1103515245 + 12345; | ||
| gSessionToken[i] = "0123456789abcdef"[(seed >> 16) & 0xF]; | ||
| seed = seed * 1664525UL + 1013904223UL; // Numerical Recipes LCG constants | ||
| gSessionToken[i] = "0123456789abcdef"[(seed >> 28) & 0xF]; // Top bits have best quality | ||
| } |
There was a problem hiding this comment.
Session token generation is still limited by a 32-bit internal state (uint32_t seed + 32-bit LCG). Even though the token is 16 hex chars (64 bits), the number of possible tokens is effectively capped at 2^32, which is relatively small for an authentication bearer token and makes brute-force guessing more feasible (especially since /api/session/check provides a fast oracle). Consider switching to a larger state (e.g., 64/128-bit) and/or using a hardware RNG/true entropy source if available on this platform for session tokens.
| #ifndef VIN_ANALOG_PIN | ||
| #define VIN_ANALOG_PIN A0 // Opta analog input pin (A0-A7, 0-10V range) | ||
| #endif | ||
| // VIN_ANALOG_PIN is defined earlier (before generateSessionToken) to avoid forward-reference. |
There was a problem hiding this comment.
The note about avoiding a “forward-reference” is a bit misleading for a macro: the actual issue is that #define expansion is order-dependent during preprocessing, so VIN_ANALOG_PIN needs to be defined before it is used. Suggest rewording to avoid confusion (e.g., “defined earlier so it’s available for preprocessing in generateSessionToken()”).
| // VIN_ANALOG_PIN is defined earlier (before generateSessionToken) to avoid forward-reference. | |
| // VIN_ANALOG_PIN is defined earlier (before generateSessionToken) so it is available for preprocessing/macro expansion. |
|
@copilot apply changes based on the comments in this thread |
Co-authored-by: dorkmo <1923070+dorkmo@users.noreply.github.com>
Done in commit
|
The "Enforce single-session login with tokens" commit introduced
generateSessionToken()at line 704 referencingVIN_ANALOG_PIN, but that macro is only#define'd at line 930 — causing a compilation error on the Server sketch.Changes
Compilation fix
Moved the
#ifndef VIN_ANALOG_PIN/#define VIN_ANALOG_PIN A0block to beforegenerateSessionToken()so the symbol is always available for preprocessing/macro expansion. Removed the duplicate define from its original location and replaced it with a clarifying comment. The function now usesVIN_ANALOG_PINdirectly, respecting any user override (e.g.#define VIN_ANALOG_PIN A1).Configurable supplementary entropy pins
Added three new
#ifndefmacros —ENTROPY_ADC_PIN_1,ENTROPY_ADC_PIN_2,ENTROPY_ADC_PIN_3— defaulting to A1, A2, A3. These can be overridden if those pins are in use for other hardware purposes.Improved entropy construction
The token generator was updated from a single-source seed to a multi-source entropy pool with a 64-bit LCG state:
VIN_ANALOG_PIN+ 3 supplementary)micros()samples bracketing the ADC reads to capture execution jittermillis()for coarser timing entropyuint64_tbefore shifting to preserve full bit widthseedHigh/seedLowhalves before combining into a 64-bit seed6364136223846793005 / 1442695040888963407) — full-period over 2^64(seed >> 60)from 64-bit state, giving 2^64 possible seeds and making brute-force infeasibleOriginal prompt
💡 You can make Copilot smarter by setting up custom instructions, customizing its development environment and configuring Model Context Protocol (MCP) servers. Learn more Copilot coding agent tips in the docs.