Repository navigation
fix: get the dashboard to load on every page - #172
Conversation
|
/review AI review started. |
Advanced AI Review
Click to expand reviewCode ReviewBase: Scope note: the committed rebuilds of I verified the core fixes against the pinned dependency versions rather than taking them on faith:
The new test is well constructed (port 0 for an ephemeral port, No high- or medium-severity issues found. Minor notes: 1. ⚪
|
![github-advanced-security[bot]](https://avatars.githubusercontent.com/in/57789?s=60&v=4){kind=small}
|
/review AI review started. |
Advanced AI Review
Click to expand reviewCode ReviewBase: Review scope: all 21 source files in the diff (893 changed lines). The generated Note: Deno is not installed in this CI environment, so the test suite and lint No high- or medium-severity issues were found. The password gate is 1. ⚪ The CID regex is duplicated from routes.ts and can silently drift
Consider moving the regex to the shared constants module that routes.ts already // src/serve/constants.ts
export const CID_REGEX = /^z[1-9A-HJ-NP-Za-km-z]{8,72}$/// src/serve/routes.ts and src/serve/dashboard-server.ts
import { CID_REGEX } from './constants.ts'2. ⚪ Credits conversion: ÷ 10¹² is the right scale; two small display edges
The PR description asks for this to be checked. Verified: Two minor display edges in the same helper:
3. ⚪ No rate limiting on the dashboard password prompt
4. ⚪ The pre-existing
|
| const samePassword = (given: Buffer, expected: Buffer): boolean => { | ||
| if (given.length !== expected.length) { | ||
| timingSafeEqual(expected, expected) | ||
| return false | ||
| } | ||
| return timingSafeEqual(given, expected) | ||
| } |
There was a problem hiding this comment.
What is going on here.... This samePassword function can be deleted and timingSafeEqual used directly, right? /cc @corrideat
There was a problem hiding this comment.
timingSafeEqual throws when the two buffers have different lengths ("Input buffers must have the same byte length"), so a password of the wrong length would end up as an error instead of a 401. samePassword just returns false in that case, and compares expected with itself so the wrong-length path takes about the same time.
There was a problem hiding this comment.
This function is unsafe as written because it leaks information about the password length. As does the conversion from string to buffer, etc.
There was a problem hiding this comment.
I'm not sure if there's a standard library function that does this. But see ctEq in chel (src/serve/routes.ts)
There was a problem hiding this comment.
You can avoid the need for this entirely by hashing expected (with a random salt) and then hashing given with the same salt each time you need to check. Then you could even use standard equality checks (you'd be revealing timing information, but not useful timing information in that case). (or, since at that point you'd have buffers, and they'll have the same size, you can also use timingSafeEqual).
var salt = random(16)
var salted = sha256(concat(salt, password))
// Then
var saltedGiven = sha256(concat(salt, given))
if (!timingSafeEqual(salted, saltedGiven)) throw new Error('Wrong password')
There was a problem hiding this comment.
Done, thanks @corrideat! Both sides are now hashed with a random salt and compared with timingSafeEqual, so samePassword is gone.
| @@ -0,0 +1,153 @@ | |||
| // What the dashboard shows, read from the server's database and from the | |||
There was a problem hiding this comment.
@corrideat This file is probably one of the only files that might need your review
taoeffect
left a comment
taoeffect
left a comment
There was a problem hiding this comment.
Excellent work @akhileshthite! 👏
My only request for changes are @corrideat's comments regarding the password check
| } | ||
|
|
||
| const readNumber = async (key: string): Promise<number> => | ||
| Number(await sbp('chelonia.db/get', key)) || 0 |
There was a problem hiding this comment.
Maybe this is fine. But, Number isn't that great here. For example, Number('0x1') (hex), Number('1.1'), etc. are values that it'd take as valid but really ought to be NaN.
| value /= 1024 | ||
| unit++ | ||
| } | ||
| return unit === 0 ? `${value} B` : `${value.toFixed(1)} ${units[unit]}` |
There was a problem hiding this comment.
The string part of this should be handled using Intl.NumberFormat (maybe with a fallback)
| const cents = (abs % PICOCREDITS_PER_CREDIT) / PICOCREDITS_PER_CENT | ||
| const shown = `${whole}.${String(cents).padStart(2, '0')}` | ||
| // A balance just below zero shows as 0.00, not -0.00 | ||
| return balance < 0n && shown !== '0.00' ? `-${shown}` : shown |
There was a problem hiding this comment.
Same with this. It should use Intl.NumberFormat
| // A contract or file ID. Shared by the API and the dashboard so the two can't | ||
| // accept different IDs. Checked as safe from ReDoS with | ||
| // <https://devina.io/redos-checker> | ||
| export const CID_REGEX = /^z[1-9A-HJ-NP-Za-km-z]{8,72}$/ |
There was a problem hiding this comment.
Do we need this? In most cases, parsing the CID is the better choice (single point of control)
There was a problem hiding this comment.
@corrideat This was removed and maybeParseCID used in src/serve/dashboard-server.ts, but in the latest commit added back as const CID_REGEX = /^z[1-9A-HJ-NP-Za-km-z]{8,72}$/ in routes.ts. What do you think, is it good there or should it also be removed there?
There was a problem hiding this comment.
It was already in routes.ts before this PR (it's there in 3.4.1). I had only moved it into constants.ts to share it with the dashboard, so once the dashboard switched to maybeParseCID, I put it back as it was. routes.ts is now the same as on master. If we want routes.ts to parse IDs too, I can do that..
There was a problem hiding this comment.
I think Chelonia apps shouldn't have to know how to do this and that we should have official exports that can be used for fast checks. Opened an issue for this: okTurtles/libcheloniajs#114
Feel free to send a PR for that and in the meantime please reference that issue in a comment here.
|
Note to self: waiting on me to release a libchel so that the CID regex can be used from that |
|
@akhileshthite 
|
taoeffect
left a comment
taoeffect
left a comment
There was a problem hiding this comment.
Fantastic work @akhileshthite! 👏 🥳
Closes #171.
Step 1: it loads on every page
./, so it was looked up relative to wherever chel was started.vue-clickawayexportsmixin, notclickaway.okTurtles.eventswas never imported.Landingname, which the router warned about. It now redirects to/.build/dist-dashboard.Step 2: live data
It only shows names, types, counts and sizes. Contract data is encrypted, and nothing reads it.
Columns the server has no data for are gone: "Created on", "Joined on", "Groups", and the user "view" button that only showed a TODO alert. Accounts and Billing are still mockups.
Two new settings under
[server]dashboardListenIP:127.0.0.1by default. The dashboard used to listen onserver.host(0.0.0.0), so opening it from another machine now needs this.dashboardAdminPassword: no default. Without it the pages load but show no data. With it, the browser asks for it. It can also be set from the environment asserver__dashboardAdminPassword.Credits are stored as picocredits, and I show them as picocredits ÷ 10¹², since chel has no conversion constant. Please check that.
build/dist-dashboardandbuild/main.jsare rebuilt with Deno 2.9.7, and only change where the source did.Testing:
deno task testpasses. New tests load every page by URL, and check the password (missing, wrong, right, and from the environment) and the data from a contract registered on the test server. I also ran it against a local chel serving TodoMVC, with three accounts, a shared list and a deleted account.AI Usage: Claude Opus 5.5 Max
Self review: Approved
Screenshots