Skip to content

Code review: router, cookies, stream, sync and date bugs #6

Open

Description

@noah-meierlabs

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)

  1. Same-host link with a // path throws: Request.ts:78 + Router.ts:166,175,267,269. https://site.ch//evil.com/x passes the host check, but pushState('//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) to pushState/replaceState. (reproduced)
  2. Cookie parsing: cookies/utils.ts:4,12. split('=') cuts values at the first = (tok=YWJj== gives YWJj, which breaks base64/JWT cookies). One malformed % cookie from another app on the domain makes decodeURIComponent throw, 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)
  3. 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)

  1. ReceiverManager retry loop has no backoff: Stream.ts:760-778. If openFn rejects 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)
  2. Receiver.close() is not idempotent: Stream.ts:656-663. A second close() on an old receiver deletes a newer receiver for the same action from the map. Return early when already closed forever, like Sender.close. (reproduced)
  3. Stream with a relative API address throws: Stream.ts:83. new URL('/api') gives "Invalid URL", although Api supports relative addresses. Resolve against location.href. (reproduced)
  4. Api.request on 204 / non-JSON bodies: Api.ts:90-96. A 204 throws Unexpected end of JSON input, and a 502 HTML page loses its status. ApiError.fromJson crashes on null/{}/numbers (ApiError.ts:51-55). (reproduced)
  5. XHR helpers differ from request(): Api.ts:138,204,237. Only === 200 counts as success (201/204 become errors), and requestTimeout drops data on GET.
  6. Api.request sends content-type: application/json on GET, which forces a CORS preflight, and it writes into the caller's opts.

chuchi-utils

  1. NonConcurrent / Latest allow two runners at once: NonConcurrent.ts:65-69, Latest.ts:39-48. done() sets running = false before the woken waiter resumes, so a synchronous start() in between also starts. Calling done() twice releases two waiters. Fix: hand the lock directly to the waiter and make done idempotent. (reproduced)
  2. range() precedence bug: index.ts:87. end - start / step should be (end - start) / step. range(0,10,2) has length 10, and range(-5,5,2) / range(5,2) throw "Invalid array length". (reproduced)
  3. searchScore: search.ts:20. It uses searchSpaceLeft for the right side, so foo/foo and foo/foobarbaz score the same. searchSpaceRight is unused (and off by one). (reproduced)
  4. SlotArray.remove twice pushes the id onto free twice, so two later pushes get the same slot: SlotArray.ts:73-78. (reproduced)

chuchi-legacy

  1. Date shifts a day west of UTC: time/Date.ts:67. new Date('2023-05-10') is UTC midnight, so in America/New_York it becomes 2023-05-09 and loses another day per toJSON → parse round trip. Parse YYYY-MM-DD into a local date. (reproduced)
  2. ISO week at year boundaries: Date.ts:117-123, DateTime.ts:113-119. It uses this.year instead of the Thursday's year: 2024-12-30 gives 53 (should be 1), and 2021-01-01 gives 0 (should be 53). (reproduced)
  3. Duration rounds after picking the unit (59.6s gives "in 60 seconds"). inViewY reports elements taller than the viewport as not visible. (reproduced)

Packaging

  1. jsr.json versions 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.
  2. jsr.json misses exports: chuchi lacks ./api, ./cookies. chuchi-legacy lacks ./dom/Context2d, ./dom/utils.
  3. The license field "MIT or Apache-2.0" isn't valid SPDX (MIT OR Apache-2.0).

Code quality

  • ApiError / GraphQlError don't extend Error (no stack, instanceof Error is false). Stream uses kind 'Other' but ApiError.newOther uses 'OTHER'.
  • usedSsrComponents (SsrComponents.ts:53) depends on Svelte 4's create_ssr_component and 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

No one assigned

    Labels

    No labels
    No labels

    Type

    No type

    Projects

    No projects

      Milestone

      No milestone

      Relationships

      None yet

      Development

      No branches or pull requests

      Issue actions