Put /upload and /delete behind a per-session token - #74
Open
chongfun wants to merge 1 commit into
Open
Conversation
`/upload`, `/delete` and `/list` answered anything on the LAN. While a wireless session was up, any device on the same network could add or delete books on the card, with no step in between. This closes that. A token is minted per serving session from the hardware RNG — the same rejection-sampled draw and the same unambiguous alphabet as the portal PSK — and every endpoint moves behind it as the first path segment: `/<token>`, `/<token>/list`, `/<token>/upload?name=...`. It is never persisted; the reset at session end retires it. The obvious cheap version — embed the token in the page served at `/` — protects nothing, because that page is exactly what an unauthorized client fetches first. The secret has to be something the user has and a stray LAN client does not, so it goes on the device: the serving screen now renders a QR of `http://<ip>/<token>`, with the same address printed under it for phones that cannot scan. That is why the alphabet excludes look-alike glyphs — someone is going to read six characters off an e-ink panel. The page itself stays a build-time constant. Its fetches derive the token from `location.pathname`, which the browser already has, so the HTML never carries the secret and templating it per request never has to exist. In `app_core::UploadToken::strip_path_prefix`, not in the server loop. It is parsing, it decides who may write to the card, and `fw` has no host tests to hold it — the same argument that moved the reader cache into its own crate. The server calls it and re-slices past the returned prefix, so the dispatch below sees exactly the paths it always did, query strings and offsets included. A refused request gets `404 Not Found`, the same answer a nonexistent path gets, before any upload machinery is touched — a wrong token must not open a storage session, and must not tell the caller it was close. Comparison is over the full length rather than short-circuiting. `the_gate_admits_exactly_the_urls_the_screen_shows` pins both directions against the concrete URLs: the four the page produces, and eleven a LAN client would reach for unaided — bare `/list`, `/upload?name=...`, wrong case, truncated and overlong segments, and the token appearing as a *later* segment (`/upload/k7mfqx`), which a check that scanned instead of anchoring would wrongly admit. It also pins that stripping leaves the query string intact, which is what keeps the delete and upload handlers' offsets valid. Mutation-checked: dropping the leading-`/` anchor and loosening the length comparison to `>` are both caught. Token minting is pinned separately — every alphabet byte must be mintable, and nothing a URL or QR payload would need escaped may be. Flash +2,140 B text; `.data` and `.bss` unchanged. `sync-flow` goldens re-blessed on both boards, deliberately: the serving screen gained the QR and the address line. Reviewed the rendered frames rather than blessing blind. **Device verification is still owed:** phone scans the QR and reaches the shelf; upload and delete work through it; the same endpoints without the token return 404 from another machine on the LAN; and the printed address is legible enough to type off the panel, which is the one thing no host test can answer.
📝 WalkthroughWalkthroughThe PR adds six-character per-session upload tokens. Firmware validates token-prefixed upload, delete, and list routes. Sync state, emulator flows, and UI rendering now carry and display the token through authenticated URLs and QR codes. ChangesUpload Token Authentication
Estimated code review effort: 4 (Complex) | ~45 minutes Sequence Diagram(s)sequenceDiagram
participant WiFiTask
participant SyncState
participant UI
participant Browser
participant UploadServer
WiFiTask->>WiFiTask: Generate session UploadToken
WiFiTask->>SyncState: Emit Serving(IP, UploadToken)
SyncState->>UI: Preserve serving token
UI->>Browser: Present token-prefixed upload URL
Browser->>UploadServer: Request authenticated route
UploadServer->>UploadServer: Validate and strip token prefix
UploadServer-->>Browser: Serve valid endpoint or return 404
Possibly related PRs
Suggested reviewers: Poem
🚥 Pre-merge checks | ✅ 4✅ Passed checks (4 passed)
✨ Finishing Touches📝 Generate docstrings
🧪 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 |
6 tasks
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
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.
Pull Request
Verification
Before requesting review, please confirm you have run the appropriate checks:
tools/check.sh fmtpassedtools/check.sh fastpassed (host Clippy and tests)tools/check.sh emulatorpassed (emulator tests and goldens)tools/check.sh firmwarepassed (firmware Clippy and release builds)tools/check.sh allpassed (required before the pull request is considered ready)Skipped checks
If you skipped any checks, please list them below and explain why:
Description
/upload,/deleteand/listanswered anything on the LAN. While a wireless session was up, any device on the same network could add or delete books on the card, with no step in between. This closes that.A token is minted per serving session from the hardware RNG — the same rejection-sampled draw and the same unambiguous alphabet as the portal PSK — and every endpoint moves behind it as the first path segment:
/<token>,/<token>/list,/<token>/upload?name=.... It is never persisted; the reset at session end retires it.The obvious cheap version — embed the token in the page served at
/— protects nothing, because that page is exactly what an unauthorized client fetches first. The secret has to be something the user has and a stray LAN client does not, so it goes on the device: the serving screen now renders a QR ofhttp://<ip>/<token>, with the same address printed under it for phones that cannot scan. That is why the alphabet excludes look-alike glyphs — someone is going to read six characters off an e-ink panel.The page itself stays a build-time constant. Its fetches derive the token from
location.pathname, which the browser already has, so the HTML never carries the secret and templating it per request never has to exist.In
app_core::UploadToken::strip_path_prefix, not in the server loop. It is parsing, it decides who may write to the card, andfwhas no host tests to hold it — the same argument that moved the reader cache into its own crate. The server calls it and re-slices past the returned prefix, so the dispatch below sees exactly the paths it always did, query strings and offsets included.A refused request gets
404 Not Found, the same answer a nonexistent path gets, before any upload machinery is touched — a wrong token must not open a storage session, and must not tell the caller it was close. Comparison is over the full length rather than short-circuiting.the_gate_admits_exactly_the_urls_the_screen_showspins both directions against the concrete URLs: the four the page produces, and eleven a LAN client would reach for unaided — bare/list,/upload?name=..., wrong case, truncated and overlong segments, and the token appearing as a later segment (/upload/k7mfqx), which a check that scanned instead of anchoring would wrongly admit. It also pins that stripping leaves the query string intact, which is what keeps the delete and upload handlers' offsets valid.Mutation-checked: dropping the leading-
/anchor and loosening the length comparison to>are both caught.Token minting is pinned separately — every alphabet byte must be mintable, and nothing a URL or QR payload would need escaped may be.
Flash +2,140 B text;
.dataand.bssunchanged.sync-flowgoldens re-blessed on both boards, deliberately: the serving screen gained the QR and the address line. Reviewed the rendered frames rather than blessing blind.Device verification is still owed: phone scans the QR and reaches the shelf; upload and delete work through it; the same endpoints without the token return 404 from another machine on the LAN; and the printed address is legible enough to type off the panel, which is the one thing no host test can answer.
Summary by CodeRabbit
New Features
Bug Fixes