Repository navigation
Code review: router, cookies, stream, sync and date bugs #6
Copy link
Copy link
Open
Description
Code review of all four packages at b4f1f2b. Points fixed by #7 and #8 have been removed. Build, check, lint and prettier all pass. Findings marked (reproduced) were reproduced with small scripts against the built dist/; the others come from reading the code.
Correctness (most severe first)
chuchi (router, cookies, ssr)
- Same-host link with a
//path throws:Request.ts:78+Router.ts:166,175,267,269.https://site.ch//evil.com/xpasses the host check, butpushState('//evil.com/x')resolves cross-origin, so it throws a SecurityError (unhandled rejection, half-updated state). Fix: pass an absolute URL (url.origin + req.uri) topushState/replaceState. (reproduced) - Cookie parsing:
cookies/utils.ts:4,12.split('=')cuts values at the first=(tok=YWJj==givesYWJj, which breaks base64/JWT cookies). One malformed%cookie from another app on the domain makesdecodeURIComponentthrow, which breaks all cookie reads (and SSR requests). An empty header gives a'' => ''entry. Fix: split on the first=, try/catch the decode, skip empty parts. (reproduced) - Cookie attributes aren't sanitised:
cookies/utils.ts:30-42.path: '/x; Domain=evil.com'is written verbatim. Reject;,,, CR/LF. (Also missing:SameSite,Expires.) (reproduced)
chuchi-core (api, stream)
ReceiverManagerretry loop has no backoff:Stream.ts:760-778. IfopenFnrejects immediately, it spins in microtasks and freezes the page (2M+ attempts, timers never fire). Unsubscribing while it is opening is also ignored (_onClose, 793), so the channel stays open with no listeners. (reproduced)Receiver.close()is not idempotent:Stream.ts:656-663. A secondclose()on an old receiver deletes a newer receiver for the same action from the map. Return early when already closed forever, likeSender.close. (reproduced)Streamwith a relative API address throws:Stream.ts:83.new URL('/api')gives "Invalid URL", althoughApisupports relative addresses. Resolve againstlocation.href. (reproduced)Api.requeston 204 / non-JSON bodies:Api.ts:90-96. A 204 throwsUnexpected end of JSON input, and a 502 HTML page loses its status.ApiError.fromJsoncrashes onnull/{}/numbers (ApiError.ts:51-55). (reproduced)- XHR helpers differ from
request():Api.ts:138,204,237. Only=== 200counts as success (201/204 become errors), andrequestTimeoutdropsdataon GET. Api.requestsendscontent-type: application/jsonon GET, which forces a CORS preflight, and it writes into the caller'sopts.
chuchi-utils
NonConcurrent/Latestallow two runners at once:NonConcurrent.ts:65-69,Latest.ts:39-48.done()setsrunning = falsebefore the woken waiter resumes, so a synchronousstart()in between also starts. Callingdone()twice releases two waiters. Fix: hand the lock directly to the waiter and makedoneidempotent. (reproduced)range()precedence bug:index.ts:87.end - start / stepshould be(end - start) / step.range(0,10,2)has length 10, andrange(-5,5,2)/range(5,2)throw "Invalid array length". (reproduced)searchScore:search.ts:20. It usessearchSpaceLeftfor the right side, sofoo/fooandfoo/foobarbazscore the same.searchSpaceRightis unused (and off by one). (reproduced)SlotArray.removetwice pushes the id ontofreetwice, so two laterpushes get the same slot:SlotArray.ts:73-78. (reproduced)
chuchi-legacy
Dateshifts a day west of UTC:time/Date.ts:67.new Date('2023-05-10')is UTC midnight, so inAmerica/New_Yorkit becomes 2023-05-09 and loses another day pertoJSON→ parse round trip. ParseYYYY-MM-DDinto a local date. (reproduced)- ISO week at year boundaries:
Date.ts:117-123,DateTime.ts:113-119. It usesthis.yearinstead of the Thursday's year: 2024-12-30 gives 53 (should be 1), and 2021-01-01 gives 0 (should be 53). (reproduced) Durationrounds after picking the unit (59.6s gives "in 60 seconds").inViewYreports elements taller than the viewport as not visible. (reproduced)
Packaging
jsr.jsonversions out of sync: chuchi-core 0.1.5 vs 0.1.1, chuchi-legacy 1.1.0 vs 1.0.2. The publish workflow now checks both against the tag, so these need to be synced before the next release of those packages.jsr.jsonmisses exports:chuchilacks./api,./cookies.chuchi-legacylacks./dom/Context2d,./dom/utils.- The license field
"MIT or Apache-2.0"isn't valid SPDX (MIT OR Apache-2.0).
Code quality
ApiError/GraphQlErrordon't extendError(no stack,instanceof Erroris false).Streamuses kind'Other'butApiError.newOtheruses'OTHER'.usedSsrComponents(SsrComponents.ts:53) depends on Svelte 4'screate_ssr_componentand silently does nothing on Svelte 5.onRequest's unsubscribe resets to the default listener even if another listener was registered later (Router.ts:103-105).
Metadata
Metadata
Assignees
Labels
No labels
Activity
[chuchi] fix cookie get after set/remove and allow remove options
[chuchi] store SsrCache entries in a Map
[chuchi] keep saving scroll positions and scroll to hash links
[core] keep the status of non-json GraphQl errors, don't modify headers
[core] don't crash in ApiError.fromJson on null, numbers or {}
[legacy] inViewY is true for elements taller than the viewport
[chuchi] don't throw on same host links with a path starting with //
[utils] fix range length, searchScore right distance and SlotArray do…
[core] don't crash in ApiError.fromJson on null, numbers or {}
[legacy] inViewY is true for elements taller than the viewport