Skip to content

fix: resolve P2-level issues found in code review (28 fixes across pkg/server/internal/edge/drivers) - #85

Open
Mcchen1008 wants to merge 6 commits into
OpenListTeam:mainfrom
Mcchen1008:fix/p2-review-findings
Open

fix: resolve P2-level issues found in code review (28 fixes across pkg/server/internal/edge/drivers)#85
Mcchen1008 wants to merge 6 commits into
OpenListTeam:mainfrom
Mcchen1008:fix/p2-review-findings

Conversation

@Mcchen1008

@Mcchen1008 Mcchen1008 commented Sep 25, 2026

Copy link
Copy Markdown

P2 review fixes: 24 issues across pkg / server / internal / edge / drivers

Following a full code review of the repository, this PR fixes the P2-level issues found (edge-condition bugs, encoding errors, status-code mistakes and latent defects). Every item below was manually verified against the source before fixing.

pkg (src/backend/pkg/)

# File Issue Fix
1 http.ts buildUrl Params were appended after #, landing in the fragment and never reaching the server Split at the fragment and append to the query
2 http.ts fetchWithTimeout Caller AbortSignal declared in FetchConfig but unconditionally overwritten by the timeout signal Combine via AbortSignal.any (guarded for old runtimes); thread signal through get/post/request
3 http.ts isSafeUrl Only 127.0.0.1.nip.io was blocked; <any-ip>.nip.io (and all of sslip.io) bypassed SSRF protection Block *.nip.io / *.sslip.io / *.xip.io entirely (admin allowHosts whitelist still takes precedence)
4 stream.ts parseRange bytes=5-x -> end=NaN escaped all checks and returned an invalid slice; suffix ranges (bytes=-N) unsupported Strict regex parsing, isNaN guards, suffix-range support, end clamped to total-1 per RFC 7233
5 csrf.ts getCookie decodeURIComponent(undefined) returned the string "undefined"; invalid % sequences threw URIError -> 500 indexOf("=") parsing + try/catch fallback to raw value
6 totp.ts verifyBackupCode Generated codes are XXXX-XXXX, but normalization only stripped whitespace - input without the hyphen could never match Normalize [-\s] on both sides
7 utils.ts formatBytes >= 1024PB returned "1024 undefined"; non-finite/negative input returned "NaN undefined" Clamp unit index; return "0 Bytes" for invalid input
8 validators.ts role schema comment/default inverted vs. UserRole enum (default 1 = GUEST; "2" mapped to ADMIN) Align to 0=GENERAL, 1=GUEST, 2=ADMIN, default 0
9 xml.ts generateWebDavXml Directory path inserted raw into <d:href> - names with & < > produced invalid PROPFIND XML (stored XML injection possible) XML-escape all hrefs

server (src/backend/server/)

# File Issue Fix
10 admin.ts Unauthorized /api/admin/* returned HTTP 200 (401 only in the body) c.json(..., 401)
11 auth.ts /login, /login/hash OTP failure returned early without recordLoginFailure - with a correct password, 6-digit TOTP could be brute-forced indefinitely, bypassing lockout Record audit log + login failure on the OTP path
12 user.ts updatePwdHandler Only verified the JWT signature: logout blacklist (jti revocation) and disabled accounts were ignored Reject revoked jti and disabled users
13 sso.ts postMessageHtml Only < > escaped; a " in the IdP-provided value broke out of the JS string literal in the inline script (XSS) Escape \, ", <>, CR/LF/U+2028/9
14 sso.ts autoRegister saveDb(db, db.env) - db.env doesn't exist (worked only via a saveDb internal fallback) Pass c.env explicitly
15 fs.ts /fs/get share branch raw_url built with unencoded sub-path - ? # % in share file names truncated/broke the URL encodeURIComponent(shareId) + encodeDownloadPath(subPath)
16 fs.ts /add_offline_download Param error returned body code: 400 with HTTP 200 Add status 400
17 fs.ts (4 sites) decodeURIComponent(File-Path header) outside try - malformed % sequences threw URIError -> 500 Shared decodeUploadHeader() helper returning null -> HTTP 400 (aligned with raw.ts)
18 fs.ts fetchArchiveBytes Passed the raw virtual path to driver.get() while physical was resolved from the base_path-joined actual path Pass actual (matches getItem / /fs/link convention)
19 webdav.ts PROPFIND Depth header read but both branches returned 207 with all children (RFC 4918 requires Depth: 0 to return the collection only) Depth: 0 -> self only
20 assets.ts resolveCdnBase Called db.get("SELECT ...") - the TS getDb() returns a plain object, so this always threw a swallowed TypeError and $version was stuck at latest Read from db.settings array

internal

# File Issue Fix
21 internal/stream/stream.ts parseRangeHeader bytes=-N -> start=NaN -> fs.createReadStream throws ERR_OUT_OF_RANGE -> /d download 500 (players seeking send suffix ranges); multi-range produced truncated slices Return null for malformed/multi/unsatisfiable ranges, support suffix ranges, clamp end
22 raw.ts (2 sites), proxy_request.ts Call sites destructured the parser result directly Fall back to full-content 200 when the Range doesn't parse (RFC-allowed); payload-limit check treats unparseable Range as full size
23 internal/archive/zip.ts writer.write()/close() not awaited - inflate errors became unhandled rejections and output could be silently truncated await both
24 internal/upload/multipart.ts snapshot received.size * chunk_size over-counted the smaller last chunk (15MB file / 10MB chunks -> 20MB) Sum actual per-chunk bytes

edge / root

# File Issue Fix
25 esa-entry.ts KV probe called bare edgeKv.get() on every request, burning 1 of 8 KV subrequests (comment claimed it was cached) 60s module-level probe cache; still probes the raw KV so failures remain visible in logs
26 esa-entry.ts delete catch {} swallowed delete failures, then invalidated caches - callers believed the delete succeeded while the value reappeared on next read Log + rethrow (consistent with put); invalidate only after success
27 middleware.js Backend whitelist missing /dav and /s3 (both mounted in src/backend/index.ts) - html-accepting GETs were rewritten to the SPA shell Add to the regex
28 loadEnv.js .env values kept paired quotes and trailing \r - JWT_SECRET="abc" loaded as an 8-char quoted secret Strip quotes + CRLF

drivers

# File Issue Fix
29 webdav/util.ts displayname not XML-unescaped (a&amp;b.txt listed literally, breaking rename/move/get); self-detection endsWith(normTarget) matched everything at root and swallowed same-named subdirs Unescape entities; strip scheme://host and match exactly
30 s3/util.ts ETag not unescaped (&quot;hex&quot; kept as-is); ListObjectsV1 fallback marker used the virtual path without a trailing slash for folders - duplicate entries / page loop Unescape then strip quotes; convert via getKey(path, isFolder)

Verification

  • tsc --noEmit: 0 new errors (2 pre-existing errors in db_cipher.test.ts also present on main)
  • test:server: 124/129 pass - identical to main baseline (same 5 pre-existing failures in seed/init/CAS areas untouched by this PR)
  • test:store 12/12, test:model 39/39, test:drivers 111/111
  • test:regress: 39/40 - identical to main baseline (getStoreConfigError export expectation pre-dates this PR)

Deliberately deferred (design-level, needs maintainer input)

  • getActualPath .. traversal vs. user base_path containment (seed.ts checks it; other fs paths rely on downstream resolvePath clamping)
  • crypt driver file format vs. rclone crypt v1 compatibility
  • KV driver list() Cloudflare/EdgeOne field compatibility (name vs key, list_complete)
  • JWT user matching id || username fallback semantics
  • multipart session ownership binding, RENAME/MOVE/COPY/DELETE permission-bit wiring, SSO state for non-OIDC platforms, CSRF middleware mounting, WebDAV Basic-Auth brute-force lockout, keyFormat atomic save, audit-log KV concurrency

@Mcchen1008
fix(pkg): harden SSRF checks, Range parsing and encoding edge cases
be187ae 
- http.ts buildUrl: append query params before the URL fragment instead
  of into it (params after '#' were never sent to the server)
- http.ts fetchWithTimeout: honor caller-provided AbortSignal via
  AbortSignal.any instead of unconditionally overwriting it; thread
  config.signal through get/post/request
- http.ts isSafeUrl: block *.nip.io and *.sslip.io entirely - only
  127.0.0.1.nip.io was blocked, so <any-ip>.nip.io bypassed SSRF checks
- stream.ts parseRange: reject NaN end, support RFC 7233 suffix ranges
  (bytes=-N) and clamp oversized end instead of dropping the request
- csrf.ts getCookie: never call decodeURIComponent(undefined) (returned
  the string "undefined") and fall back to the raw value on invalid
  percent-encoding instead of throwing URIError (500)
- totp.ts verifyBackupCode: strip hyphens on both sides - generated
  backup codes are XXXX-XXXX, so user input without the hyphen never
  matched
- utils.ts formatBytes: clamp unit index for >= 1024PB and return
  "0 Bytes" for non-finite/negative input instead of "NaN undefined"
- validators.ts: align role schema with the UserRole enum (0=GENERAL,
  1=GUEST, 2=ADMIN); the previous comment/default were inverted, which
  would default-create a GUEST and map "2" to ADMIN
- xml.ts generateWebDavXml: XML-escape hrefs - directory names with
  & < > produced invalid PROPFIND multistatus (and allowed stored XML
  injection via crafted directory names)
@Mcchen1008
fix(server): auth hardening and correct HTTP status codes
41e046d 
- admin.ts: return a real HTTP 401 for unauthorized /api/admin/* requests;
  c.json({code:401}) without the status arg returned HTTP 200
- auth.ts /login and /login/hash: record login failures + audit log on OTP
  failure too - previously a correct password allowed unlimited TOTP
  guessing, bypassing the per-IP/account lockout
- user.ts updatePwdHandler: reject revoked tokens (jti in logout blacklist)
  and disabled accounts, aligning with authUserFromReq semantics
- sso.ts postMessageHtml: escape backslash, quotes and line terminators -
  only escaping <> let a value containing '"' break out of the JS string
  literal in the inline postMessage script (XSS via IdP-provided fields)
- sso.ts autoRegister: persist with the caller's c.env instead of the
  non-existent db.env property (only worked via a saveDb internal fallback)
@Mcchen1008
fix(server): fs/webdav/assets edge-case fixes
aac7e79 
- fs.ts /fs/get share branch: URL-encode shareId and sub-path in raw_url;
  file names containing ? # % truncated the URL or broke requests
- fs.ts add_offline_download: return HTTP 400 (was body code 400 with
  HTTP 200)
- fs.ts: decode File-Path/Upload-Path headers via a safe helper and
  return 400 on malformed percent-encoding instead of an uncaught
  URIError -> 500 (4 call sites, aligned with raw.ts behavior)
- fs.ts fetchArchiveBytes: pass the base_path-resolved actual path to
  driver.get(), matching the getItem//fs/link convention; the raw
  virtual path resolved archives to the wrong object for base_path users
- webdav.ts PROPFIND: honor the Depth header - Depth: 0 now returns only
  the collection itself per RFC 4918 (previously depth was read but both
  branches returned the full child list)
- assets.ts resolveCdnBase: read the version setting from db.settings
  (TS getDb() has no Go-style db.get(sql) API); the old call always threw
  a swallowed TypeError so $version was stuck at 'latest'
@Mcchen1008
fix(internal): robust Range parsing, zip extract, multipart snapshot
43ca7f8 
- internal/stream parseRangeHeader: return null for malformed/multi-range/
  unsatisfiable Range instead of NaN garbage; support RFC 7233 suffix
  ranges (bytes=-N) and clamp end to size-1. Previously 'Range: bytes=-500'
  produced start=NaN and crashed fs.createReadStream with
  ERR_OUT_OF_RANGE (500) on /d downloads
- raw.ts: fall back to a full-content 200 when the Range header does not
  parse (RFC allows ignoring Range) instead of streaming an invalid slice
- proxy_request.ts exceedsProxyPayloadLimit: treat unparseable Range as
  full-size transfer for the payload-limit check
- archive/zip.ts extractZipEntry: await writer.write()/close() so inflate
  errors propagate through the await chain instead of becoming unhandled
  rejections with truncated output
- upload/multipart.ts snapshot: sum actual per-chunk bytes - the last
  chunk is smaller than chunk_size when size is not a multiple, so
  received.size * chunk_size over-counted (e.g. 15MB file / 10MB chunks
  reported 20MB)
@Mcchen1008
fix(edge): esa-entry KV probe/delete, SPA middleware whitelist, loadEnv
e085391 
- esa-entry.ts: cache the KV probe result for 60s (module level) instead
  of burning one KV subrequest per request - ESA allows only 8 per
  request; keep probing the raw EdgeKV so failures stay visible
- esa-entry.ts delete: rethrow on failure (consistent with put) and only
  invalidate caches after success - swallowing errors made callers
  believe a delete succeeded while the value would reappear on next get
- middleware.js: add /dav and /s3 to the backend path whitelist; both are
  mounted routes, and html-accepting GETs to them were rewritten to the
  SPA index.html instead of hitting the backend
- loadEnv.js: strip paired quotes and CRLF remnants from .env values so
  JWT_SECRET="abc" no longer yields an 8-char quoted secret
@Mcchen1008
fix(drivers): webdav XML parsing and s3 list edge cases
e5fe2b7 
- webdav/util.ts: unescape XML entities in displayname so file names
  like 'a&amp;b.txt' are listed correctly (previously broke follow-up
  rename/move/get); self-detection now strips the scheme://host prefix
  and uses exact matching - the old bare endsWith() matched any entry
  when the target was the root and swallowed same-named subdirectories
- s3/util.ts: unescape ETag entities before stripping quotes (ETag is
  serialized as &quot;hex&quot; in ListObjects XML); ListObjectsV1
  fallback marker now converts the last entry via getKey() - the raw
  virtual path lacked the trailing slash for folders and restarted the
  same page, duplicating entries or looping
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

No reviews

Assignees

No one assigned

Labels

None yet

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

1 participant

@Mcchen1008