Skip to content

docs(office): document all env vars and add size limits guide - #15414

Open
chrip wants to merge 1 commit into
masterfrom
feature/nextcloud-office-docs
Open

docs(office): document all env vars and add size limits guide#15414
chrip wants to merge 1 commit into
masterfrom
feature/nextcloud-office-docs

Conversation

@chrip

@chrip chrip commented Aug 5, 2026

Copy link
Copy Markdown
Contributor

Document every environment variable supported by the Document Server Docker image in the installation page. Previously only a subset was listed.

Add a 'Size limits' section with a 200 MB PPTX example showing how the four size-gate variables interact at different stages (nginx upload, temp file buffer, converter download, uncompressed XML size).

Related to Euro-Office/DocumentServer#237 and Euro-Office/server#35

Assisted-by: OpenCode:qwen3.6-27b

@github-actions

github-actions Bot commented Aug 5, 2026

Copy link
Copy Markdown
Contributor

📖 Documentation Preview

🔍 Open preview →

📄 1 changed documentation page

Last updated: Thu, 06 Aug 2026 10:01:34 GMT

@chrip
chrip force-pushed the feature/nextcloud-office-docs branch from 84fbe4b to 75c25c3 Compare August 6, 2026 08:51
Document every environment variable supported by the Document Server
Docker image in the installation page. Previously only a subset was listed.

Add a 'Size limits' section with a 200 MB PPTX example showing how the
four size-gate variables interact at different stages (nginx upload, temp
file buffer, converter download, uncompressed XML size).

Assisted-by: OpenCode:qwen3.6-27b
Signed-off-by: Christoph Schaefer <christoph.schaefer@nextcloud.com>
@chrip
chrip force-pushed the feature/nextcloud-office-docs branch from 75c25c3 to 1eba328 Compare August 6, 2026 09:39
@chrip
chrip requested review from moodyjmz and rikled August 6, 2026 10:03

@moodyjmz moodyjmz left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

TL;DR: Most of the new env-var table checks out against the entrypoint script and the two upstream PRs it's based on, but the new "Size limits" walkthrough gets the request flow wrong for its own headline scenario (opening a stored document), and USE_UNAUTHORIZED_STORAGE is mis-described in a way that understates its security impact. Requesting changes on those two before merge; the completeness gaps below can be a fast follow.

Full review, with exact lines

1. "Size limits" section's core narrative is backwards

installation_docker.rst#L192-L228

Opening a document already stored in Nextcloud is a server-side download, not a browser upload:

  • canvasService.downloadFile (canvasservice.js#L1744) is what actually fetches the document. It reads FILECONVERTER_MAX_DOWNLOAD_BYTES at L1771 and passes it into utils.downloadUrlPromise at L1849 — that's stage 3/4 territory, not stage 1/2.
  • The route is registered as app.get('/downloadfile/:docid', canvasService.downloadFile) with no body parser at all — see the route table in server.js#L206-L244. rawFileParser (the bodyParser.raw({ limit: limits_tempfile_upload }) that MAX_FILE_SIZE controls, L206-L211) is only attached to /command, /converter, /upload/:docid, /savefile/:docid, /downloadfile/:docid POST — image inserts, save-back, WOPI callbacks. Nothing in the "open a stored document" path.
  • NGINX_CLIENT_MAX_BODY_SIZE gates inbound POST bodies to nginx — same problem, no inbound POST body exists when the Document Server is the one fetching the file.

So of the four stages walked through in the doc, only 3 and 4 (FILECONVERTER_MAX_DOWNLOAD_BYTES, FILECONVERTER_INPUT_LIMIT_UNCOMPRESSED) apply to the "user opens a 200 MB PPTX" scenario as written. Following the guide's stage 1/2 advice for that scenario won't do anything.

2. USE_UNAUTHORIZED_STORAGE description is wrong, and understates the risk

installation_docker.rst#L161-L163 documents it as "Allow fetching documents from HTTP (non-TLS) storage."

It actually flows into requestDefaults.rejectUnauthorized, cloned into the HTTPS agent options for the exact same downloadUrlPromise call used to fetch documents — see utils.js#L345-L355, specifically httpsAgentOptions = {...https.globalAgent.options, ...options} at L354. rejectUnauthorized: false is the standard Node.js TLS option for skipping certificate-chain validation — it has no bearing on plain HTTP (no certificate is ever presented on an HTTP connection either way). Flip this thinking you're permitting a legacy plaintext endpoint, and you've disabled cert validation on every HTTPS storage connection too — an unlabelled MITM exposure.

3. The PR's stated goal — "document every environment variable" — isn't quite true

entrypoint.sh still reads several operator-facing vars the table skips (all in entrypoint.sh):
SSL_CERTIFICATE_PATH/SSL_KEY_PATH/SSL_DHPARAM_PATH (L349-L367), SECURE_LINK_SECRET (L118-L127), METRICS_HOST/PORT/PREFIX (L45-L47), REDIS_SERVER_USER/DB, AMQP_URI/VHOST, NGINX_WORKER_CONNECTIONS, ADMINPANEL_ENABLED/EXAMPLE_ENABLED, DS_LOG_LEVEL.

Worth prioritizing the SSL ones: this PR does document SSL_VERIFY_CLIENT and both ONLYOFFICE_HTTPS_HSTS_* vars, but all three only take effect inside the if [ -n "$SSL_CERTIFICATE_PATH" ] && [ -n "$SSL_KEY_PATH" ] gate at L349-L367. Document those three without the two vars that gate them, and a reader can set all three and see zero effect.

4. Minor — undocumented edge-case behavior (low priority)

  • FILECONVERTER_INPUT_LIMIT_UNCOMPRESSED replaces the entire inputLimits array rather than patching one entry (entrypoint.sh#L261-L270). Harmless today (only four format groups exist in default.json), but a footgun if a fifth type group is ever added upstream.
  • FILECONVERTER_MAX_DOWNLOAD_BYTES is silently ignored (stderr warning only) if it isn't a plain integer (entrypoint.sh#L255-L260) — worth a one-line callout.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants