Hardening: API request handling, random IDs, and plugin loading

[JohnMcLear]

A second small hardening pass, covering API request handling, random-ID generation, and plugin loading. Companion to #7905.

Changes

• pad_utils.randomString — generate the random IDs (author/session/readonly) via crypto.getRandomValues (a CSPRNG) instead of Math.random, with rejection sampling to avoid modulo bias. Output format/length is unchanged.
• OAuth2Provider — constant-time password comparison and a uniform failure delay on the OIDC interaction login; look the user up by own property only.
• API.appendChatMessage — require the pad to already exist (getPadSafe), consistent with the other content API methods.
• RestAPI (/api/2) — forward only the authorization header instead of merging all request headers into the API field set (matches the openapi.ts handler).
• LinkInstaller — validate plugin dependency names before using them to build filesystem paths.
• admin file server — return a generic error and log the detail server-side.

⚠️ Behaviour / breaking changes

1. appendChatMessage now returns padID does not exist for a non-existent pad instead of silently creating it. Callers that relied on the implicit pad creation must call createPad (or createGroupPad) first. This aligns it with setText/getText/etc.
2. /api/2 no longer exposes arbitrary request headers as API parameters. Only authorization is forwarded. This only affects a caller that passed an API parameter via an HTTP header of the same name (not a documented usage); send such values as query/body parameters instead.
3. Failed OIDC interaction logins now take ~1s (uniform failure delay). Behavioural only.

The remaining changes (randomString, LinkInstaller, admin error text) are non-breaking.

Testing

• Backend specs covering the touched areas pass (chat, api, jwtAdminClaim, LinkInstaller, author-token, webaccess, tokenTransfer, OIDCAdapter).
• randomString validated in a real browser via the author_token_cookie chromium spec (HttpOnly token, stable authorID across reload, unique per context).

🤖 Generated with Claude Code

[qodo-code-review]

Qodo reviews are paused for this user.

Troubleshooting steps vary by plan Learn more →

On a Teams plan?
Reviews resume once this user has a paid seat and their Git account is linked in Qodo.
Link Git account →

Using GitHub Enterprise Server, GitLab Self-Managed, or Bitbucket Data Center?
These require an Enterprise plan - Contact us
Contact us →

[qodo-free-for-open-source-projects]

Code Review by Qodo

🐞 Bugs (0) 📘 Rule violations (0) 📎 Requirement gaps (0) 🎨 UX issues (0)

1. Auth fallback inconsistent ✓ Resolved 🐞 Bug ≡ Correctness

Description

/api/2 only copies headers.authorization into fields.authorization when the field is
null/undefined, but the OpenAPI router falls back to the header when the field is any falsy value.
This can cause the same request to authenticate differently depending on which router handles it
(e.g., body/query includes authorization: "").

Code

src/node/handler/RestAPI.ts[R1474-1480]

+    // Merge with clear precedence: body > query > path params, matching the
+    // openapi.ts handler. Forward only the authorization header explicitly
+    // instead of merging all request headers into the field set.
+    const fields = Object.assign({}, params, query, formData);
+    if (headers && headers.authorization && fields.authorization == null) {
+      fields.authorization = headers.authorization;
+    }

Evidence

The /api/2 router uses a nullish check (fields.authorization == null) before copying the header,
whereas the OpenAPI router uses fields.authorization || headers.authorization, which treats ''
as absent and will use the header. This difference is visible directly in the two routing
implementations.

src/node/handler/RestAPI.ts[1474-1480]
src/node/hooks/express/openapi.ts[754-760]

Agent prompt

The issue below was found during a code review. Follow the provided context and guidance below and implement a solution

## Issue description
`src/node/handler/RestAPI.ts` forwards the `authorization` header only when `fields.authorization == null`, but the OpenAPI router (`src/node/hooks/express/openapi.ts`) uses a falsy check (`fields.authorization || headers.authorization`). This creates inconsistent behavior across API entrypoints.
## Issue Context
- RestAPI: `fields.authorization` is set only if it is `null`/`undefined`.
- OpenAPI router: header is used as a fallback if `fields.authorization` is falsy (including empty string).
## Fix Focus Areas
- src/node/handler/RestAPI.ts[1474-1480]
### Suggested change
Update RestAPI to match the OpenAPI behavior, for example:
- `if (headers?.authorization && !fields.authorization) fields.authorization = headers.authorization;`
(or `fields.authorization ||= headers.authorization`),
so that empty-string (and other falsy) values don’t suppress the header.

ⓘ Copy this prompt and use it to remediate the issue with your preferred AI generation tools

---

[qodo-free-for-open-source-projects]

Review Summary by Qodo

Harden API request handling, token generation, and plugin loading

✨ Enhancement 🐞 Bug fix

Walkthroughs

Description

• Replace Math.random with crypto.getRandomValues for secure ID generation
• Implement constant-time password comparison in OAuth2Provider
• Add uniform failure delay to OIDC login failures
• Require pad existence before appending chat messages
• Forward only authorization header in /api/2 requests
• Validate plugin dependency names before filesystem operations
• Return generic error messages from admin file server

Diagram

flowchart LR
  A["Security Improvements"] --> B["Token Generation"]
  A --> C["Authentication"]
  A --> D["API Validation"]
  B --> B1["crypto.getRandomValues<br/>rejection sampling"]
  C --> C1["Constant-time comparison"]
  C --> C2["Failure delay"]
  D --> D1["Pad existence check"]
  D --> D2["Header filtering"]
  D --> D3["Dependency validation"]

Loading

File Changes

1. src/static/js/pad_utils.ts ✨ Enhancement +15/-4

Use CSPRNG for secure random ID generation

• Replace Math.random() with crypto.getRandomValues() for CSPRNG-based ID generation
• Implement rejection sampling to avoid modulo bias
• Maintain unchanged output format and length

src/static/js/pad_utils.ts

---

2. src/node/security/OAuth2Provider.ts ✨ Enhancement +30/-10

Add constant-time comparison and failure delay

• Add constantTimeEquals() function using SHA-256 digests and crypto.timingSafeEqual
• Implement uniform 1-second failure delay on failed OIDC logins
• Change user lookup to use Object.prototype.hasOwnProperty instead of array mapping
• Add explicit break statement after failed login to prevent grant processing

src/node/security/OAuth2Provider.ts

---

3. src/node/db/API.ts 🐞 Bug fix +5/-1

Require pad existence before appending messages

• Add getPadSafe() call to appendChatMessage to require pad existence
• Return padID does not exist error for non-existent pads instead of creating them
• Align behavior with other content API methods like setText and getText

src/node/db/API.ts

---

View more (3)

4. src/node/handler/RestAPI.ts ✨ Enhancement +7/-1

Filter request headers in /api/2 endpoint

• Change field merging to exclude request headers by default
• Forward only authorization header explicitly when present
• Maintain precedence order: body > query > path params
• Match behavior with openapi.ts handler

src/node/handler/RestAPI.ts

---

5. src/static/js/pluginfw/LinkInstaller.ts ✨ Enhancement +17/-0

Validate plugin dependency names before use

• Add isValidDependencyName() method to validate npm package names
• Check for path separators, "..", and invalid characters before filesystem operations
• Add validation calls in addSubDependency() and linkDependency() methods
• Log and skip invalid dependency names

src/static/js/pluginfw/LinkInstaller.ts

---

6. src/node/hooks/express/admin.ts ✨ Enhancement +4/-1

Return generic error from admin file server

• Log detailed filesystem error server-side with full path and error details
• Return generic error message to client instead of exposing filesystem details
• Improve security by preventing information disclosure

src/node/hooks/express/admin.ts

---

[SamTV12345]

Or we just throw an error. That way somebody can send chat messages to random pads and they are created.

[JohnMcLear]

getPadSafe(padID, true) already throws padID does not exist when the pad is missing (API.ts:1029) — it does not create it — so chat can no longer materialise random pads. It's the same existence check getText/setText/getHTML use, and it also validates the padID string/format.

Happy to swap it for an explicit throw new CustomError('padID does not exist', 'apierror') if you'd rather not fetch the pad here — functionally identical, just let me know which you'd prefer to merge.

[SamTV12345]

Ah okay. Thought getPadSafe would also create if missing. Then let's be more explicit with an error like you suggested throw new CustomError('padID does not exist', 'apierror')

[JohnMcLear]

Done — switched to the explicit throw new CustomError('padID does not exist', 'apierror') (guarded by padManager.doesPadExists), so no pad is fetched or created. Chat spec still green.