Deploy to PHP (shared hosting) - #1019
Conversation
✅ Deploy Preview for livecodes ready!
To edit notification comments on pull requests, go to your Netlify project configuration. |
hatemhosny
changed the title
![github-advanced-security[bot]](https://avatars.githubusercontent.com/in/57789?s=60&v=4){kind=small}
|
|
||
| - name: Deploy to FTP server | ||
| if: ${{ steps.check.outputs.proceed == 'true' }} | ||
| uses: SamKirkland/FTP-Deploy-Action@v4.4.0 |
|
|
||
| - name: Deploy via SSH | ||
| if: ${{ steps.check.outputs.proceed == 'true' }} | ||
| uses: SamKirkland/web-deploy@v1 |
| cache-dependency-path: "**/package-lock.json" | ||
|
|
||
| - name: Install dependencies | ||
| run: npm ci |
| - name: Check if FTP credentials are configured | ||
| id: check | ||
| run: | | ||
| if [ -z "${{ secrets.FTP_HOST }}" ]; then |
|
|
||
| - name: Install dependencies | ||
| if: ${{ steps.check.outputs.proceed == 'true' }} | ||
| run: npm ci |
| // the database user may not have the CREATE privilege | ||
| // (e.g. when the database was pre-created by the hosting provider) | ||
| } | ||
| $pdo->exec("USE `$dbName`"); |
|
|
||
| function sendCorsHeaders(): void | ||
| { | ||
| header('Access-Control-Allow-Origin: *'); |
| array $headers = [], | ||
| int $timeout = 5, | ||
| ): array { | ||
| $ch = curl_init($url); |
| array $headers = [], | ||
| int $timeout = 5, | ||
| ): array { | ||
| $ch = curl_init($url); |
| ]; | ||
|
|
||
| header('Content-Type: application/json; charset=utf-8'); | ||
| header('Access-Control-Allow-Origin: *'); |
|
Size Change: 0 B Total Size: 1.1 MB ℹ️ View Unchanged
|
Deploying livecodes with
|
| Latest commit: |
bfaedb9
|
| Status: | ✅ Deploy successful! |
| Preview URL: | https://b19617e8.livecodes.pages.dev |
| Branch Preview URL: | https://deploy-to-php.livecodes.pages.dev |
![pullfrog[bot]](https://avatars.githubusercontent.com/in/1768019?s=60&v=4){kind=small}
There was a problem hiding this comment.
Caution
This PR introduces a regression in src/livecodes/services/allowed-origin.ts — localhost is no longer recognized as a valid origin (a duplicate localpen.pages.dev line replaced it). This breaks origin-based feature gating on local development setups.
Reviewed changes — adds PHP/MySQL self-hosting support for shared hosting with complete server-side service implementations (share, broadcast, CORS proxy, sandbox, oEmbed, dynamic meta tags), refactors CI/CD deploy workflows into per-target files with guard conditions, and updates docs across multiple pages.
- PHP server implementation (
server/php/) — ports all Cloudflare Pages functions and the Node broadcast server to PHP + MySQL, using HTTP polling instead of WebSockets for broadcast state - CI/CD workflow refactor — splits
deploy.ymlintodeploy-ghpages.ymland addsdeploy-php.yml, with guard variables (PHP_ENV,VPS_ENV) preventing conflicting deployments - Build system changes — adds
HOST_NAMEenv var, changes default ports to 443, switches toJSON.stringifyfor env var quoting, addscopy-phpbuild script - Client-side service changes — allows self-hosted domains in
allowedOrigin, strips default:443from broadcast URLs, reorders sandbox URL priority - Documentation — new PHP setup guide, deployment reference, sidebar updates
🚨 localhost origin validation broken in client-side allowedOrigin
The src/livecodes/services/allowed-origin.ts diff accidentally replaced matchesDomain(hostname, 'localhost') with a duplicate matchesDomain(hostname, 'localpen.pages.dev'), meaning localhost is no longer recognized as a valid origin. This affects all allowedOrigin() callers on local development (broadcast viewer pages, share URL resolution, oEmbed).
Technical details
# localhost origin validation broken
## Affected sites
- `src/livecodes/services/allowed-origin.ts:21` — duplicate `localpen.pages.dev` should be `'localhost'`
## Required outcome
- Restore the `matchesDomain(hostname, 'localhost')` check in the client-side `allowedOrigin` function
## Suggested approach
Replace line 21:
```typescript
matchesDomain(hostname, 'localpen.pages.dev') ||
```
with:
```typescript
matchesDomain(hostname, 'localhost') ||
```ℹ️ Nitpicks
-
server/php/README.md:5references.github/workflows/php-deploy.ymlbut the actual file is nameddeploy-php.yml. -
src/livecodes/services/broadcast.ts:4— the.replace(':443', '')string replacement is fragile: it would mangle a hostname or path containing:443(e.g.:8443/→8/). Consider.replace(/:443\//, '/')or conditionally omitting the port when it equals'443'.
DeepSeek Pro (free via Pullfrog for OSS) (Kimi K2 not used — the program covers this model; add its provider key to run your pick) ｜ 𝕏
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
WalkthroughAdds PHP/MySQL shared-hosting support with generated deployment artifacts, FTP/SSH workflows, broadcast and API endpoints, sandbox serving, configuration helpers, and documentation. Deployment guards and self-hosted origin, broadcast, and sandbox environment handling are also updated. ChangesPHP self-hosting and deployment
Estimated code review effort: 5 (Critical) | ~120 minutes Sequence Diagram(s)sequenceDiagram
participant Publisher
participant BroadcastIndex
participant MySQL
participant BroadcastViewer
Publisher->>BroadcastIndex: POST channel update
BroadcastIndex->>MySQL: Create or update channel
MySQL-->>BroadcastIndex: Persist channel state
BroadcastViewer->>BroadcastIndex: Poll channel state
BroadcastIndex->>MySQL: Read channel result and data
MySQL-->>BroadcastIndex: Return broadcast state
BroadcastIndex-->>BroadcastViewer: JSON state response
Possibly related PRs
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✨ Finishing Touches🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 19
Note
Due to the large number of review comments, Critical, Major severity comments were prioritized as inline comments.
🟡 Minor comments (6)
docs/docs/advanced/php.mdx-108-110 (1)
108-110: 📐 Maintainability & Code Quality | 🟡 Minor | ⚡ Quick winHyphenate “third-party.”
Line 110 should say “third-party sites.”
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@docs/docs/advanced/php.mdx` around lines 108 - 110, Update the oEmbed description to hyphenate “third-party” when describing third-party sites, leaving the surrounding link and wording unchanged.Source: Linters/SAST tools
server/php/README.md-5-5 (1)
5-5: 📐 Maintainability & Code Quality | 🟡 Minor | ⚡ Quick winWrong workflow filename.
The workflow added in this PR is
.github/workflows/deploy-php.yml, notphp-deploy.yml.📝 Fix
-These files are copied to the `build/` directory after the app is built (`npm run copy-php`), and are deployed via FTP/SSH using the GitHub workflow `.github/workflows/php-deploy.yml`. +These files are copied to the `build/` directory after the app is built (`npm run copy-php`), and are deployed via FTP/SSH using the GitHub workflow `.github/workflows/deploy-php.yml`.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@server/php/README.md` at line 5, Update the deployment workflow reference in the README text to use `.github/workflows/deploy-php.yml` instead of `.github/workflows/php-deploy.yml`; leave the surrounding build and deployment description unchanged.server/php/vendor/lzstring.php-120-120 (1)
120-120: 🩺 Stability & Availability | 🟡 Minor | ⚡ Quick winDocument the
mbstringrequirement.
server/php/vendor/lzstring.phpcallsmb_convert_encoding(...), so this server copy now needs thembstringextension in addition topdo_mysql/curl. Add it to the PHP server requirements inserver/php/README.md, or provide aniconv('UTF-16BE', 'UTF-8', $result)fallback whenmb_convert_encodingis missing.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@server/php/vendor/lzstring.php` at line 120, Document the new mbstring extension requirement for the PHP server in server/php/README.md, alongside the existing pdo_mysql and curl requirements, since the lzstring implementation calls mb_convert_encoding. Do not add a fallback unless choosing instead to remove this dependency.server/php/config.sample.php-20-38 (1)
20-38: 📐 Maintainability & Code Quality | 🟡 Minor | ⚡ Quick winSample omits two supported keys.
getConfig()inserver/php/inc/config.php(lines 24-36) also loadsSELF_HOSTED_SHAREandSELF_HOSTED_BROADCAST. Since the docs here sayconfig.phpcan be created manually, list them too.📝 Suggested addition
// optional: comma-separated user tokens for the broadcast server (empty = no auth) 'BROADCAST_TOKENS' => '', + + // optional: enable self-hosted share / broadcast services + 'SELF_HOSTED_SHARE' => '', + 'SELF_HOSTED_BROADCAST' => '', ];🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@server/php/config.sample.php` around lines 20 - 38, Update the sample configuration array to include the supported SELF_HOSTED_SHARE and SELF_HOSTED_BROADCAST keys loaded by getConfig(), with appropriate empty/default values and comments indicating their purpose.server/php/broadcast/assets/code.html-87-87 (1)
87-87: 🎯 Functional Correctness | 🟡 Minor | ⚡ Quick winUse subdirectory-safe broadcast URLs.
state.phpis polled with absolute/broadcast/state.php, while.htaccessrewrites broadcast endpoints only at domain root (^/?$,^channels/...). Appendingbroadcast/index.phpinstead ofstate.phpalso differs from the documented API path. Pass a base prefix from PHP or derive it from the channel page URL before making these requests.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@server/php/broadcast/assets/code.html` at line 87, Update the polling request in the broadcast client around fetch and its channel-page URL handling to construct a subdirectory-safe endpoint prefix instead of hardcoding /broadcast/state.php. Derive the base prefix from the current PHP-rendered page or pass it from PHP, then append the documented state.php API path without changing the channel encoding.server/php/api/cors.php-46-53 (1)
46-53: 🎯 Functional Correctness | 🟡 Minor | ⚡ Quick win
statusTextis always empty, so this fallback message is dead code.
fetchUrl()(utils.php) always returns'statusText' => '', so the ternary at Line 51 always echoes'Bad Gateway!'regardless of the actual upstream status. Either populatestatusTextinfetchUrl(e.g. via a reason-phrase map orcurl_getinfo) or drop the ternary.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@server/php/api/cors.php` around lines 46 - 53, The error response in the CORS handler relies on `fetchUrl()` returning a meaningful `statusText`, but that function currently always returns an empty string. Update `fetchUrl()` to populate `statusText` from the upstream response, such as via curl response metadata or a status-reason mapping, while preserving the existing `Bad Gateway!` fallback when no message is available.
🧹 Nitpick comments (9)
.github/workflows/deploy-ghpages.yml (1)
37-43: 🔒 Security & Privacy | 🔵 Trivial | ⚡ Quick winScope the generated GitHub App token to the minimum permission needed.
Without a
permissions:input,create-github-app-tokenissues a token carrying every permission granted to the app's installation, not just thecontents: writethis workflow uses to push togh-pages.🔒 Proposed fix
- name: Generate GitHub App token if: ${{ steps.check-secrets.outputs.available == 'true' }} uses: actions/create-github-app-token@v1 id: generate-token with: app-id: ${{ secrets.CI_APP_ID }} private-key: ${{ secrets.CI_APP_PRIVATE_KEY }} + permission-contents: write🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In @.github/workflows/deploy-ghpages.yml around lines 37 - 43, Update the generate-token step using actions/create-github-app-token so its permissions input grants only contents: write, preserving the existing conditional execution and token configuration.Source: Linters/SAST tools
.github/workflows/deploy-php.yml (1)
38-42: 🔒 Security & Privacy | 🔵 Trivial | ⚡ Quick winMissing
persist-credentials: falseon new checkout steps. All three new checkout steps default to persisting the checkout credential in.git/configthrough subsequent build steps that run third-party lifecycle scripts, even though none of these workflows need the ambient git credential afterward (each authenticates its deploy action separately).
.github/workflows/deploy-php.yml#L38-L42: addpersist-credentials: falseto thedeploy-ftpjob's checkout..github/workflows/deploy-php.yml#L145-L149: addpersist-credentials: falseto thedeploy-sshjob's checkout..github/workflows/deploy-ghpages.yml#L45-L49: addpersist-credentials: false; the workflow already re-authenticates its push via the separately generatedDEPLOY_TOKEN.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In @.github/workflows/deploy-php.yml around lines 38 - 42, Disable checkout credential persistence by adding persist-credentials: false to the checkout steps in .github/workflows/deploy-php.yml lines 38-42 and 145-149, and .github/workflows/deploy-ghpages.yml lines 45-49; leave the existing branch selection and separate deployment authentication unchanged.Source: Linters/SAST tools
server/php/inc/db.php (1)
30-58: 🚀 Performance & Scalability | 🔵 Trivial | 🏗️ Heavy liftSchema DDL runs on every request.
Each request that touches the DB pays a
CREATE DATABASE IF NOT EXISTS, aUSE, and twoCREATE TABLE IF NOT EXISTSround trips before any real work — noticeable on shared hosting where connection latency dominates. Consider running the bootstrap lazily (only after a query fails with "table doesn't exist") or once per deploy behind a marker file/setup endpoint.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@server/php/inc/db.php` around lines 30 - 58, The database bootstrap in the initialization flow runs on every request; move the CREATE DATABASE, USE, and CREATE TABLE statements out of the normal request path and invoke them only through a one-time setup mechanism, such as a deploy marker or explicit setup endpoint. Ensure regular requests use an already-initialized schema without issuing these DDL round trips.server/php/broadcast/index.php (3)
65-69: 🔒 Security & Privacy | 🔵 Trivial | 💤 Low valueCompare tokens with
hash_equals.
channelToken(and the user token on line 34) are secrets; use a constant-time comparison and an explicit string cast so a non-string payload cannot trigger anArray to string conversionnotice.♻️ Suggested change
- if (($body['channelToken'] ?? '') !== $existing['channel_token']) { + $providedToken = $body['channelToken'] ?? ''; + if (!is_string($providedToken) || !hash_equals((string) $existing['channel_token'], $providedToken)) {🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@server/php/broadcast/index.php` around lines 65 - 69, Update the channelToken validation and the user-token comparison near line 34 to use hash_equals with explicit string casts for both operands, preserving the existing unauthorized response and exit behavior while preventing non-string payload conversion notices.
43-47: 🚀 Performance & Scalability | 🔵 Trivial | ⚡ Quick winEviction sweep on every broadcast POST, over an unindexed column.
Broadcasts are posted continuously while a user types, and each one issues a
DELETE ... WHERE last_accessed < ?that cannot use an index (broadcast_channelsinserver/php/inc/db.phponly indexeschannel). Add an index onlast_accessedand/or only sweep probabilistically (e.g. ~1% of requests) to keep the write path cheap.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@server/php/broadcast/index.php` around lines 43 - 47, Add an index for broadcast_channels.last_accessed in the schema setup, and change the eviction sweep around the broadcast POST handler to run only probabilistically (approximately 1% of requests). Preserve the existing 20-minute cutoff and deletion behavior when the sweep is selected.
49-53: 🩺 Stability & Availability | 🔵 Trivial | 💤 Low valueValidate the client-supplied
channelformat.
(string) $body['channel']emits anArray to string conversionwarning when a client posts an array/object. A cheappreg_match('/^[a-z0-9]{1,11}$/', ...)guard (matchinggenerateId()inserver/php/inc/utils.phplines 16-25) rejects malformed input with a 400 before touching the DB.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@server/php/broadcast/index.php` around lines 49 - 53, Update the channel parsing around $newChannel and $channel to validate client-supplied values before casting or using them: accept only strings matching generateId()’s /^[a-z0-9]{1,11}$/ format, and return an HTTP 400 response for invalid scalar, array, or object inputs before any database work. Preserve generated IDs for empty or omitted channels.server/php/broadcast/assets/code.html (1)
85-108: 🚀 Performance & Scalability | 🔵 Trivial | ⚡ Quick winAdd backoff for failed polls.
On errors (network loss, DB down, 5xx) the loop keeps hitting
state.phpevery 1.5s indefinitely, which is exactly the load pattern shared hosts throttle. An exponential backoff capped at ~30s keeps the viewer alive without hammering the server.♻️ Suggested change
+ let delay = 1500; const poll = async () => { try { const res = await fetch('/broadcast/state.php?channel=' + encodeURIComponent(channel), { cache: 'no-store', }); @@ if (newConfig !== lastConfig) { lastConfig = newConfig; await loadConfig(result, data); } + delay = 1500; } catch (e) { - // retry on next interval + delay = Math.min(delay * 2, 30000); } - setTimeout(poll, 1500); + setTimeout(poll, delay); };🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@server/php/broadcast/assets/code.html` around lines 85 - 108, Update the poll loop around the async poll function to apply exponential backoff after fetch or processing errors, capping the retry delay at approximately 30 seconds; reset the delay after a successful poll, while preserving the existing 1.5-second interval for healthy polling and the 404 disconnect behavior.server/php/index.php (1)
104-110: 🎯 Functional Correctness | 🔵 Trivial | 💤 Low valueFallback path can emit a partial page.
If a
Throwableis raised afterecho $modifiedBody(line 98) — e.g. insideheader()on an already-flushed buffer — the catch block appends the entire unmodified$originalBody, producing two concatenated documents. Guard with a flag (or move theechoto the very end of thetry) so the fallback only runs when nothing was written.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@server/php/index.php` around lines 104 - 110, Update the try/catch flow around the modified-body output and catch block to track whether a response body has already been written, or defer echoing until all fallible operations complete. Only emit the fallback $originalBody when no output was written, preventing it from being appended after $modifiedBody; preserve the existing error data updates.server/php/.htaccess (1)
11-13: 🩺 Stability & Availability | 🔵 Trivial | ⚡ Quick winGuard the sandbox catch-all against re-entrant rewrites.
In per-directory
.htaccesscontext, Apache can re-processsandbox/index.phpon the internal redirect, soRewriteRule ^ sandbox/index.php [L]can exceedLimitInternalRecursionand returnAH00124. Add a condition that skips the already-rewritten URI.♻️ Suggested guard
# sandbox (separate origin): requests for the sandbox subdomain RewriteCond %{HTTP_HOST} ^sandbox\. [NC] + RewriteCond %{REQUEST_URI} !^/sandbox/index\.php$ RewriteRule ^ sandbox/index.php [L]🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@server/php/.htaccess` around lines 11 - 13, Update the sandbox rewrite block associated with the `HTTP_HOST` condition to skip requests whose URI is already `sandbox/index.php` before applying the catch-all `RewriteRule`; preserve the existing routing for other sandbox-subdomain requests.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In @.github/workflows/deploy-docker.yml:
- Around line 33-34: Secure the remote checkout commands by preventing the
branch input from being interpolated as unquoted shell syntax in the SSH script.
Update the deployment workflow around git checkout and pull to pass the selected
branch as a safely quoted or environment-backed argument, while preserving the
default to main and ensuring crafted branch values cannot execute additional
commands.
In @.github/workflows/deploy-php.yml:
- Around line 56-69: Replace direct `${{ vars.PHP_ENV }}` interpolation inside
each `node -e` script with an environment variable supplied through the step’s
`env:` block, then read that variable from `process.env` in the script. Apply
this change to all four occurrences across the PHP and SSH deployment jobs while
preserving the existing parsing and GITHUB_ENV writes.
- Around line 79-115: Update the shared config-generation logic used by the
“Create server config” steps in both deploy-php and deploy-ssh, preferably by
extracting it into a reusable script. Escape backslashes and single quotes when
serializing values into PHP single-quoted strings, and omit unset secrets rather
than writing the literal string “undefined”; ensure both jobs invoke the same
corrected implementation.
In `@scripts/utils.js`:
- Around line 69-74: Update the hostName assignment to apply the 'null' fallback
before converting or otherwise retaining the environment value, matching the
existing SANDBOX_HOST_NAME pattern. Ensure an unset HOST_NAME produces the
literal 'null' sentinel while preserving configured host names.
In `@server/php/.htaccess`:
- Around line 20-21: Add module-independent access denial for the inc/ and
vendor/ directories, using directory-level authorization such as nested
.htaccess files with Require all denied or an equivalent FilesMatch rule. Keep
the existing RewriteRule as defense in depth, and ensure the denial applies even
when an earlier rewrite rule matches or mod_rewrite is unavailable.
In `@server/php/api/cors.php`:
- Around line 14-21: Replace the ad hoc origin condition in the request guard
with the existing allowedOrigin() helper, ensuring requests are rejected when
the Origin header is absent or fails the helper’s exact host/subdomain
validation. Preserve the existing GET/POST method restriction and 403 response
behavior.
In `@server/php/api/share.php`:
- Line 24: Update the query prepared in the share-loading function to compare
the primary-key column directly and preserve indexed lookup, normalizing the
input parameter before binding if case-insensitive IDs are required. Apply the
same approach used to fix importProject(), removing LOWER() from the SQL column
expression.
- Around line 44-51: Add an explicit maximum size check for $rawBody immediately
after reading php://input and before json_decode or any INSERT path, rejecting
payloads above the configured MEDIUMTEXT-safe limit with HTTP 413 (or the
endpoint’s established client-error response). Ensure the limit applies to the
raw request bytes, including otherwise valid JSON, and preserve normal
processing for payloads within the limit.
In `@server/php/broadcast/assets/result.html`:
- Around line 29-33: Update the result iframe’s sandbox configuration to prevent
srcdoc content from combining allow-same-origin with allow-scripts. Remove
allow-same-origin unless same-origin access is required, or route the broadcast
result through a separate isolated origin consistent with the other sandbox path
while preserving the remaining permissions.
In `@server/php/broadcast/channel.php`:
- Around line 34-42: Update the HTML substitution in the channel response flow
to HTML-escape the value returned by getRequestOrigin() before passing it to
str_replace, while preserving the existing AppUrl placeholder replacement and
response behavior.
In `@server/php/broadcast/index.php`:
- Around line 86-91: Handle a false return from json_encode in the dataJson
assignment before checking for '[]' or calling strlen, and normalize encoding
failures to the existing valid empty-object representation '{}'. Preserve the
current size limit and empty-array conversion so storedData is always valid
JSON.
In `@server/php/inc/db.php`:
- Around line 22-28: Update the PDO initialization around the $options array and
new PDO call to set a finite PDO::ATTR_TIMEOUT, then wrap the connection attempt
in exception handling that logs server-side details without exposing the DSN,
host, port, or credentials to the client. Preserve exception mode while
returning a safe generic failure response and terminating the request on
connection failure.
In `@server/php/inc/utils.php`:
- Around line 49-96: Harden fetchUrl by allowing only http and https URLs, then
validate the hostname and resolved destination before curl_init; reject
loopback, private, reserved, link-local, and other non-public IPv4/IPv6
addresses using DNS-rebinding-safe resolution and ensure the address curl
connects to is the validated one. Return the existing failed-response shape for
rejected targets while preserving normal public HTTP(S) requests.
- Around line 27-32: Update getRequestOrigin() to validate HTTP_HOST against the
configured getConfig('HOST_NAME') or allowedOrigin() before using it as the
origin, falling back to the trusted configured host when invalid. In
channel.php, HTML-encode the validated AppUrl replacement at the template
injection point before rendering it.
- Around line 154-187: The self-hosted lookup in importProject should remain
index-friendly by avoiding LOWER() on the share.id column. Use an exact id
comparison, or lowercase the parameter once and compare it directly when IDs are
stored lowercase, while preserving case-insensitive behavior as appropriate.
In `@server/php/index.php`:
- Around line 63-87: Escape the client-controlled $origin before interpolating
it into HTML in the modifiedBody replacements, including the href replacement
and the livecodes content replacement. Reuse the existing encodeHTML function,
matching the escaping already applied to $currentUrl, while preserving the
current URL construction and replacement behavior.
In `@server/php/sandbox/index.php`:
- Around line 43-48: Update the path-traversal guard in the `$filePath`
validation to compare against `$sandboxDir` with a trailing directory separator,
ensuring only paths within the sandbox directory pass while preserving the
existing missing-file and 404 handling.
In `@src/livecodes/services/allowed-origin.ts`:
- Around line 21-22: Update the allowlist condition in the allowed-origin logic
to include matchesDomain(hostname, 'localhost') alongside the existing
localpen.pages.dev entry. Remove the duplicate localpen.pages.dev check while
preserving the HOST_NAME condition and keeping behavior aligned with
functions/allowed-origin.ts.
In `@src/livecodes/services/broadcast.ts`:
- Around line 2-4: Update the self-hosted branch of getUrl to use port 3030 when
BROADCAST_PORT is unset, then apply the existing removal of the default HTTPS
port 443 so the URL remains valid in both cases.
---
Minor comments:
In `@docs/docs/advanced/php.mdx`:
- Around line 108-110: Update the oEmbed description to hyphenate “third-party”
when describing third-party sites, leaving the surrounding link and wording
unchanged.
In `@server/php/api/cors.php`:
- Around line 46-53: The error response in the CORS handler relies on
`fetchUrl()` returning a meaningful `statusText`, but that function currently
always returns an empty string. Update `fetchUrl()` to populate `statusText`
from the upstream response, such as via curl response metadata or a
status-reason mapping, while preserving the existing `Bad Gateway!` fallback
when no message is available.
In `@server/php/broadcast/assets/code.html`:
- Line 87: Update the polling request in the broadcast client around fetch and
its channel-page URL handling to construct a subdirectory-safe endpoint prefix
instead of hardcoding /broadcast/state.php. Derive the base prefix from the
current PHP-rendered page or pass it from PHP, then append the documented
state.php API path without changing the channel encoding.
In `@server/php/config.sample.php`:
- Around line 20-38: Update the sample configuration array to include the
supported SELF_HOSTED_SHARE and SELF_HOSTED_BROADCAST keys loaded by
getConfig(), with appropriate empty/default values and comments indicating their
purpose.
In `@server/php/README.md`:
- Line 5: Update the deployment workflow reference in the README text to use
`.github/workflows/deploy-php.yml` instead of
`.github/workflows/php-deploy.yml`; leave the surrounding build and deployment
description unchanged.
In `@server/php/vendor/lzstring.php`:
- Line 120: Document the new mbstring extension requirement for the PHP server
in server/php/README.md, alongside the existing pdo_mysql and curl requirements,
since the lzstring implementation calls mb_convert_encoding. Do not add a
fallback unless choosing instead to remove this dependency.
---
Nitpick comments:
In @.github/workflows/deploy-ghpages.yml:
- Around line 37-43: Update the generate-token step using
actions/create-github-app-token so its permissions input grants only contents:
write, preserving the existing conditional execution and token configuration.
In @.github/workflows/deploy-php.yml:
- Around line 38-42: Disable checkout credential persistence by adding
persist-credentials: false to the checkout steps in
.github/workflows/deploy-php.yml lines 38-42 and 145-149, and
.github/workflows/deploy-ghpages.yml lines 45-49; leave the existing branch
selection and separate deployment authentication unchanged.
In `@server/php/.htaccess`:
- Around line 11-13: Update the sandbox rewrite block associated with the
`HTTP_HOST` condition to skip requests whose URI is already `sandbox/index.php`
before applying the catch-all `RewriteRule`; preserve the existing routing for
other sandbox-subdomain requests.
In `@server/php/broadcast/assets/code.html`:
- Around line 85-108: Update the poll loop around the async poll function to
apply exponential backoff after fetch or processing errors, capping the retry
delay at approximately 30 seconds; reset the delay after a successful poll,
while preserving the existing 1.5-second interval for healthy polling and the
404 disconnect behavior.
In `@server/php/broadcast/index.php`:
- Around line 65-69: Update the channelToken validation and the user-token
comparison near line 34 to use hash_equals with explicit string casts for both
operands, preserving the existing unauthorized response and exit behavior while
preventing non-string payload conversion notices.
- Around line 43-47: Add an index for broadcast_channels.last_accessed in the
schema setup, and change the eviction sweep around the broadcast POST handler to
run only probabilistically (approximately 1% of requests). Preserve the existing
20-minute cutoff and deletion behavior when the sweep is selected.
- Around line 49-53: Update the channel parsing around $newChannel and $channel
to validate client-supplied values before casting or using them: accept only
strings matching generateId()’s /^[a-z0-9]{1,11}$/ format, and return an HTTP
400 response for invalid scalar, array, or object inputs before any database
work. Preserve generated IDs for empty or omitted channels.
In `@server/php/inc/db.php`:
- Around line 30-58: The database bootstrap in the initialization flow runs on
every request; move the CREATE DATABASE, USE, and CREATE TABLE statements out of
the normal request path and invoke them only through a one-time setup mechanism,
such as a deploy marker or explicit setup endpoint. Ensure regular requests use
an already-initialized schema without issuing these DDL round trips.
In `@server/php/index.php`:
- Around line 104-110: Update the try/catch flow around the modified-body output
and catch block to track whether a response body has already been written, or
defer echoing until all fallible operations complete. Only emit the fallback
$originalBody when no output was written, preventing it from being appended
after $modifiedBody; preserve the existing error data updates.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository UI
Review profile: CHILL
Plan: Pro Plus
Run ID: 03d16012-943f-4d47-8c65-c91bb4819fd6
📒 Files selected for processing (40)
.dockerignore.github/workflows/deploy-docker.yml.github/workflows/deploy-ftp.yml.github/workflows/deploy-ghpages.yml.github/workflows/deploy-php.yml.github/workflows/deploy-ssh.yml.github/workflows/deploy.ymldocs/docs/advanced/docker.mdxdocs/docs/advanced/php.mdxdocs/docs/advanced/services.mdxdocs/docs/contribution/README.mddocs/docs/contribution/build-system.mdxdocs/docs/contribution/deployment.mdxdocs/docs/features/self-hosting.mdxdocs/sidebars.tsfunctions/allowed-origin.tspackage.jsonscripts/copy-php.jsscripts/utils.jsserver/php/.htaccessserver/php/README.mdserver/php/api/cors.phpserver/php/api/share.phpserver/php/broadcast/assets/code.htmlserver/php/broadcast/assets/index.htmlserver/php/broadcast/assets/result.htmlserver/php/broadcast/channel.phpserver/php/broadcast/index.phpserver/php/broadcast/state.phpserver/php/config.sample.phpserver/php/inc/config.phpserver/php/inc/db.phpserver/php/inc/utils.phpserver/php/index.phpserver/php/oembed.phpserver/php/sandbox/index.phpserver/php/vendor/lzstring.phpsrc/livecodes/services/allowed-origin.tssrc/livecodes/services/broadcast.tssrc/livecodes/services/sandbox.ts
💤 Files with no reviewable changes (1)
- .github/workflows/deploy.yml
| # block direct web access to server-side includes and config | ||
| RewriteRule ^(inc|vendor)/ - [F,L] |
There was a problem hiding this comment.
🔒 Security & Privacy | 🟠 Major | ⚡ Quick win
inc/+vendor/ protection is bypassable and mod_rewrite-dependent.
Two gaps:
- On the sandbox host, line 13 matches first with
[L], so this deny rule never runs. - If
mod_rewriteis unavailable (theIfModulesilently skips the whole block),inc/config.phpbecomes directly requestable; a PHP parse-error or misconfigured handler then leaks MySQL credentials.
Prefer a module-independent deny in addition to the rewrite rule (e.g. server/php/inc/.htaccess with Require all denied, or <FilesMatch> deny in this file).
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@server/php/.htaccess` around lines 20 - 21, Add module-independent access
denial for the inc/ and vendor/ directories, using directory-level authorization
such as nested .htaccess files with Require all denied or an equivalent
FilesMatch rule. Keep the existing RewriteRule as defense in depth, and ensure
the denial applies even when an earlier rewrite rule matches or mod_rewrite is
unavailable.
| getUrl: () => | ||
| process.env.SELF_HOSTED_BROADCAST === 'true' | ||
| ? `https://${location.hostname}:${process.env.BROADCAST_PORT || '3030'}` | ||
| ? `https://${location.hostname}:${process.env.BROADCAST_PORT}/`.replace(':443', '') |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟠 Major | ⚡ Quick win
Keep a fallback broadcast port.
When BROADCAST_PORT is unset, Line 4 now returns https://<host>:undefined/, breaking self-hosted broadcast connections. Restore the previous 3030 fallback before removing port 443.
Proposed fix
- ? `https://${location.hostname}:${process.env.BROADCAST_PORT}/`.replace(':443', '')
+ ? `https://${location.hostname}:${process.env.BROADCAST_PORT || '3030'}/`.replace(
+ ':443',
+ '',
+ )📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| getUrl: () => | |
| process.env.SELF_HOSTED_BROADCAST === 'true' | |
| ? `https://${location.hostname}:${process.env.BROADCAST_PORT || '3030'}` | |
| ? `https://${location.hostname}:${process.env.BROADCAST_PORT}/`.replace(':443', '') | |
| getUrl: () => | |
| process.env.SELF_HOSTED_BROADCAST === 'true' | |
| ? `https://${location.hostname}:${process.env.BROADCAST_PORT || '3030'}/`.replace( | |
| ':443', | |
| '', | |
| ) |
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@src/livecodes/services/broadcast.ts` around lines 2 - 4, Update the
self-hosted branch of getUrl to use port 3030 when BROADCAST_PORT is unset, then
apply the existing removal of the default HTTPS port 443 so the URL remains
valid in both cases.
There was a problem hiding this comment.
Actionable comments posted: 8
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
server/php/inc/db.php (1)
35-43: 🩺 Stability & Availability | 🟡 Minor | ⚡ Quick winDo not suppress every database-creation failure.
catch (Throwable $e)swallows unexpectedPDO::exec()errors as well as the expected “pre-created database” case. Catch only the expected permission failure (PDOExceptionwith code1044/ SQLSTATE42000), and rethrow anything else with the original exception preserved.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@server/php/inc/db.php` around lines 35 - 43, Update the database-creation try/catch around $pdo->exec() to catch PDOException specifically and suppress only the expected access-denied condition identified by error code 1044 or SQLSTATE 42000. Rethrow all other exceptions, preserving the original exception, and retain the existing behavior for successful creation and an empty $dbName.
🧹 Nitpick comments (1)
server/php/inc/db.php (1)
47-63: 🚀 Performance & Scalability | 🔵 Trivial | 🏗️ Heavy liftMove schema creation out of the request path.
getDb()is used by the share and broadcast endpoints, so every request executes two DDL statements. This adds latency and metadata-lock contention under concurrent traffic. Prefer a deployment/first-run migration and keep request-time initialization limited to selecting the configured database.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@server/php/inc/db.php` around lines 47 - 63, Remove the share and broadcast_channels CREATE TABLE statements from the request-time getDb() initialization path. Move schema creation into the deployment or first-run migration mechanism, while keeping getDb() limited to selecting and returning the configured database connection.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In @.github/workflows/deploy-docker.yml:
- Line 36: The deploy script currently interpolates VPS_ENV into the remote
shell, allowing its contents to be interpreted as shell syntax. Update the
appleboy/ssh-action configuration to expose the value through env.VPS_ENV_RAW
and include VPS_ENV_RAW in envs, then write it using printf '%s\n'
"$VPS_ENV_RAW" instead of the interpolated echo in the deployment script.
In @.github/workflows/deploy-php.yml:
- Line 11: Update both actions/checkout configurations in deploy-php.yml,
including the deploy-ftp block at lines 11-11 and deploy-ssh block at lines
149-153, to use the same event-aware ref expression, such as
github.event.inputs.branch || github.ref_name. Remove the main fallback and
restore the missing ref in deploy-ssh.
- Around line 149-153: Update both “Checkout code” steps in the workflow to set
actions/checkout@v4’s persist-credentials option to false, ensuring subsequent
build and deploy commands do not retain Git credentials.
- Around line 57-64: Update the “Load PHP_ENV configuration” workflow step to
append SELF_HOSTED=true to process.env.GITHUB_ENV before npm run build:app,
using appendFileSync and preserving the existing DOCS_BASE_URL and PHP_ENV
handling.
- Around line 59-64: Remove the stale duplicate const env declaration from the
node -e script in the deploy workflow, keeping the initial env derived from
PHP_ENV_RAW and all subsequent uses of that variable unchanged.
In `@server/php/api/share.php`:
- Around line 45-50: Update the request-body handling around $maxBodyBytes and
file_get_contents('php://input') to reject a declared Content-Length exceeding
the limit before reading any body, then read at most $maxBodyBytes + 1 bytes
when the length is unknown or chunked. Preserve the existing 413 response for
bodies that exceed the limit after the bounded read.
In `@server/php/inc/db.php`:
- Around line 29-33: Update getDb so the PDO connection and all database
bootstrap steps, including USE and table creation, use a local connection
variable; assign it to the static cache only after every setup operation
succeeds, ensuring failed initialization is not reused by later calls.
In `@src/livecodes/services/broadcast.ts`:
- Line 4: Update the broadcast URL construction near the protocol/hostname
expression to default BROADCAST_PORT to 3030 when unset or empty, derive the
optional port suffix separately, and append it only when the effective port is
not 443. Remove the nested template that can produce an undefined or empty port.
---
Outside diff comments:
In `@server/php/inc/db.php`:
- Around line 35-43: Update the database-creation try/catch around $pdo->exec()
to catch PDOException specifically and suppress only the expected access-denied
condition identified by error code 1044 or SQLSTATE 42000. Rethrow all other
exceptions, preserving the original exception, and retain the existing behavior
for successful creation and an empty $dbName.
---
Nitpick comments:
In `@server/php/inc/db.php`:
- Around line 47-63: Remove the share and broadcast_channels CREATE TABLE
statements from the request-time getDb() initialization path. Move schema
creation into the deployment or first-run migration mechanism, while keeping
getDb() limited to selecting and returning the configured database connection.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository UI
Review profile: CHILL
Plan: Pro Plus
Run ID: 2c4c64b9-653e-4cd5-a80c-c77e6737dbcf
📒 Files selected for processing (12)
.github/workflows/deploy-docker.yml.github/workflows/deploy-php.ymlscripts/utils.jsserver/php/README.mdserver/php/api/cors.phpserver/php/api/share.phpserver/php/broadcast/index.phpserver/php/inc/db.phpserver/php/index.phpserver/php/sandbox/index.phpsrc/livecodes/services/allowed-origin.tssrc/livecodes/services/broadcast.ts
| git pull origin main | ||
| git checkout "$DEPLOY_BRANCH" | ||
| git pull origin "$DEPLOY_BRANCH" | ||
| echo "${{ vars.VPS_ENV }}" > .env |
There was a problem hiding this comment.
🔒 Security & Privacy | 🟠 Major | ⚡ Quick win
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo "== workflow excerpt =="
if [ -f .github/workflows/deploy-docker.yml ]; then
nl -ba .github/workflows/deploy-docker.yml | sed -n '1,90p'
else
fd -a deploy-docker.yml .github 2>/dev/null || true
fi
echo
echo "== search VPS_ENV and ssh-action usages =="
rg -n "VPS_ENV|appleboy/ssh-action|envs:|shell:" .github || true
echo
echo "== behavioral shell expansion probe =="
python3 - <<'PY'
import re, subprocess
text = '''
envs: DEPLOY_BRANCH
script: |
\t echo "${{ vars.VPS_ENV }}" > .env\n'''
print("workflow value interpolation happens before action sends script; Python only models the source string structure, not GitHub runner substitution.")
PYRepository: live-codes/livecodes
Length of output: 221
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo "== workflow excerpt =="
if [ -f .github/workflows/deploy-docker.yml ]; then
awk '{printf "%6d\t%s\n", NR, $0}' .github/workflows/deploy-docker.yml | sed -n '1,90p'
else
find .github -type f -name 'deploy-docker.yml' -print 2>/dev/null || true
fi
echo
echo "== search VPS_ENV and ssh-action usages =="
rg -n "VPS_ENV|appleboy/ssh-action|envs:|shell:" .github || true
echo
echo "== read-only YAML structure probe =="
python3 - <<'PY'
from pathlib import Path
p = Path(".github/workflows/deploy-docker.yml")
if p.exists():
lines = p.read_text().splitlines()
for i,l in enumerate(lines,1):
if "vars.VPS_ENV" in l or "VPS_ENV_RAW" in l or "appleboy/ssh-action" in l or "envs:" in l:
print(f"{i}: {l}")
PYRepository: live-codes/livecodes
Length of output: 2290
Do not interpolate VPS_ENV into the remote shell script.
{{ vars.VPS_ENV }} is expanded before appleboy/ssh-action sends the script, so values containing command substitutions, backticks, or shell syntax can execute on the VPS instead of being written safely to .env. Add VPS_ENV_RAW to envs and write it with printf '%s\n' "$VPS_ENV_RAW"; set env.VPS_ENV_RAW: ${{ vars.VPS_ENV }}.
🧰 Tools
🪛 zizmor (1.28.0)
[info] 36-36: code injection via template expansion (template-injection): may expand into attacker-controllable code
(template-injection)
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In @.github/workflows/deploy-docker.yml at line 36, The deploy script currently
interpolates VPS_ENV into the remote shell, allowing its contents to be
interpreted as shell syntax. Update the appleboy/ssh-action configuration to
expose the value through env.VPS_ENV_RAW and include VPS_ENV_RAW in envs, then
write it using printf '%s\n' "$VPS_ENV_RAW" instead of the interpolated echo in
the deployment script.
Source: Linters/SAST tools
| push: | ||
| branches: | ||
| - main | ||
| - deploy-to-php |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟠 Major | ⚡ Quick win
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo "== file excerpt =="
sed -n '1,220p' .github/workflows/deploy-php.yml 2>/dev/null || true
echo
echo "== git diff stat/name-status =="
git diff --stat HEAD~1..HEAD 2>/dev/null || true
git diff -- .github/workflows/deploy-php.yml 2>/dev/null | sed -n '1,240p' || true
echo
echo "== repository refs for relevant files (static) =="
if [ -f .github/workflows/deploy-php.yml ]; then
echo "exists"
else
echo "missing"
fiRepository: live-codes/livecodes
Length of output: 8863
Use one event-aware deployment ref across both jobs.
Both deploy-ftp checkout configs fall back to main even when deploy-to-php is pushed, and deploy-ssh no longer checks out any branch. Use one expression such as github.event.inputs.branch || github.ref_name in both actions/checkout ref settings.
📍 Affects 1 file
.github/workflows/deploy-php.yml#L11-L11(this comment).github/workflows/deploy-php.yml#L149-L153
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In @.github/workflows/deploy-php.yml at line 11, Update both actions/checkout
configurations in deploy-php.yml, including the deploy-ftp block at lines 11-11
and deploy-ssh block at lines 149-153, to use the same event-aware ref
expression, such as github.event.inputs.branch || github.ref_name. Remove the
main fallback and restore the missing ref in deploy-ssh.
| - name: Checkout code | ||
| if: ${{ steps.check.outputs.proceed == 'true' }} | ||
| uses: actions/checkout@v4 | ||
| # with: | ||
| # ref: ${{ github.event.inputs.branch || 'main' }} |
There was a problem hiding this comment.
🔒 Security & Privacy | 🟠 Major | ⚡ Quick win
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo "== workflow excerpt =="
sed -n '1,220p' .github/workflows/deploy-php.yml | cat -n
echo
echo "== action pins and npm/build usage =="
rg -n "actions/checkout|npm ci|npm install|npm run|persist-credentials|permissions:|GITHUB_TOKEN|git push|php" .github/workflows/deploy-php.ymlRepository: live-codes/livecodes
Length of output: 12572
🌐 Web query:
actions/checkout v4 persist-credentials default true README
💡 Result:
For the actions/checkout action, the persist-credentials input defaults to true [1][2]. This setting determines whether the authentication token or SSH key used to fetch the repository is configured in the local git config [2][3]. When enabled (the default), this allows your workflow scripts to run authenticated git commands (such as git push or git fetch) against the repository [2][4]. The action automatically cleans up these credentials during the post-job phase [2][4]. If you wish to prevent these credentials from being configured in the local git config, you can explicitly set persist-credentials: false in your workflow step [2][4]. Note that newer versions of the action have improved security by storing these credentials in a separate file under $RUNNER_TEMP rather than directly in the repository's.git/config [4][5][6].
Citations:
- 1: https://github.com/actions/checkout/blob/v4/action.yml
- 2: https://github.com/actions/checkout/blob/v4/README.md
- 3: https://github.com/actions/checkout/blob/v4.1.1/action.yml
- 4: https://github.com/actions/checkout/?tab=readme-ov-file
- 5: https://github.com/actions/checkout/tree/v6.0.0
- 6: actions/checkout@v5.0.1...v6.0.0
Disable checkout credential persistence for both builds.
actions/checkout@v4 persists credentials by default, so the subsequent npm ci, npm run build:app, npm run copy-php, config generation, and deploy steps run with Git credentials available. Set persist-credentials: false on both Checkout code steps unless authenticated Git commands are later needed in the same job.
🧰 Tools
🪛 zizmor (1.28.0)
[warning] 149-153: credential persistence through GitHub Actions artifacts (artipacked): does not set persist-credentials: false
(artipacked)
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In @.github/workflows/deploy-php.yml around lines 149 - 153, Update both
“Checkout code” steps in the workflow to set actions/checkout@v4’s
persist-credentials option to false, ensuring subsequent build and deploy
commands do not retain Git credentials.
Source: Linters/SAST tools
| $maxBodyBytes = 2 * 1024 * 1024; // adjust as appropriate | ||
| if ($rawBody !== false && strlen($rawBody) > $maxBodyBytes) { | ||
| http_response_code(413); | ||
| echo 'Payload Too Large!'; | ||
| exit; | ||
| } |
There was a problem hiding this comment.
🩺 Stability & Availability | 🟠 Major | ⚡ Quick win
Bound the request read before checking its size.
The 2 MiB check runs only after file_get_contents('php://input') has already buffered the entire request, so oversized requests can still exhaust PHP memory before returning 413. Reject an oversized Content-Length early and read at most $maxBodyBytes + 1 bytes for chunked/unknown-length requests.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@server/php/api/share.php` around lines 45 - 50, Update the request-body
handling around $maxBodyBytes and file_get_contents('php://input') to reject a
declared Content-Length exceeding the limit before reading any body, then read
at most $maxBodyBytes + 1 bytes when the length is unknown or chunked. Preserve
the existing 413 response for bodies that exceed the limit after the bounded
read.
| try { | ||
| $pdo = new PDO("mysql:host=$host;port=$port;charset=utf8mb4", $user, $password, $options); | ||
| } catch (PDOException $e) { | ||
| throw new RuntimeException('Database connection failed.'); | ||
| } |
There was a problem hiding this comment.
🩺 Stability & Availability | 🟡 Minor | ⚡ Quick win
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo "Files:"
git ls-files | rg '(^|/)db\.php$|server/php/inc' || true
echo
echo "db.php outline:"
if [ -f server/php/inc/db.php ]; then
ast-grep outline server/php/inc/db.php --view expanded || true
echo
echo "db.php contents:"
cat -n server/php/inc/db.php
fi
echo
echo "Search getDb/getConnection usages:"
rg -n "getDb|getConnection|class .*Db|new .*Db|PDO|PDOException|RuntimeException|USE |CREATE TABLE|mysqli|DB::|db::" server/php -S || trueRepository: live-codes/livecodes
Length of output: 4498
Cache the connection only after bootstrap succeeds.
$pdo is stored in the static cache immediately after connection, before USE and table creation complete. If bootstrap fails after getDb() returns once, later calls reuse the partially initialized PDO and skip setting up the database/tables. Keep connection initialization and setup local, and assign the static cache only after all setup succeeds.
🧰 Tools
🪛 GitHub Check: SonarCloud Code Analysis
[warning] 32-32: Define and throw a dedicated exception instead of using a generic one.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@server/php/inc/db.php` around lines 29 - 33, Update getDb so the PDO
connection and all database bootstrap steps, including USE and table creation,
use a local connection variable; assign it to the static cache only after every
setup operation succeeds, ensuring failed initialization is not reused by later
calls.
| getUrl: () => | ||
| process.env.SELF_HOSTED_BROADCAST === 'true' | ||
| ? `https://${location.hostname}:${process.env.BROADCAST_PORT || '3030'}` | ||
| ? `https://${location.hostname}${process.env.BROADCAST_PORT !== '443' ? `:${process.env.BROADCAST_PORT}` : ''}/` |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟠 Major | ⚡ Quick win
Restore the default broadcast port.
When BROADCAST_PORT is unset, this expression generates https://<host>:undefined/; an empty value also produces an invalid trailing colon. Restore the 3030 fallback and derive the port suffix separately to avoid the nested template.
Proposed fix
getUrl: () =>
- process.env.SELF_HOSTED_BROADCAST === 'true'
- ? `https://${location.hostname}${process.env.BROADCAST_PORT !== '443' ? `:${process.env.BROADCAST_PORT}` : ''}/`
- : 'https://vps.livecodes.io:3030/',
+ (() => {
+ const broadcastPort = process.env.BROADCAST_PORT || '3030';
+ const portSuffix = broadcastPort === '443' ? '' : `:${broadcastPort}`;
+ return process.env.SELF_HOSTED_BROADCAST === 'true'
+ ? `https://${location.hostname}${portSuffix}/`
+ : 'https://vps.livecodes.io:3030/';
+ })(),🧰 Tools
🪛 GitHub Check: SonarCloud Code Analysis
[warning] 4-4: Extract this nested ternary operation into an independent statement.
[warning] 4-4: Refactor this code to not use nested template literals.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@src/livecodes/services/broadcast.ts` at line 4, Update the broadcast URL
construction near the protocol/hostname expression to default BROADCAST_PORT to
3030 when unset or empty, derive the optional port suffix separately, and append
it only when the effective port is not 443. Remove the nested template that can
produce an undefined or empty port.
Source: Linters/SAST tools
There was a problem hiding this comment.
Important
The FTP job's "Load PHP_ENV configuration" step has a duplicate const env declaration that would throw a SyntaxError and break deployment.
Reviewed changes — 13 targeted fixes addressing prior review feedback: restored localhost origin validation, replaced fragile :443 string replacement with conditional port logic, HTML-encoded origin in index.php, switched CORS endpoint to use allowedOrigin() helper, added payload size limit to share endpoint, hardened json_encode in broadcast, added PDO timeout and error wrapping, fixed sandbox path traversal guard, fixed shell injection in Docker deploy, fixed HOST_NAME env handling, fixed README workflow filename, added value escaping to config generation, and fixed SSH job's PHP_ENV loading.
src/livecodes/services/allowed-origin.ts— restoredlocalhostcheck and addedHOST_NAMEsupportsrc/livecodes/services/broadcast.ts— replaced.replace(':443', '')with conditional port inclusionserver/php/index.php— HTML-encodes$originin meta-tag and oembed link replacementsserver/php/api/cors.php— switched from ad-hocstriposcheck toallowedOrigin()helperserver/php/api/share.php— added$maxBodyBytessize limit with 413 responseserver/php/broadcast/index.php— addedJSON_INVALID_UTF8_SUBSTITUTEflag andis_string()guardserver/php/inc/db.php— addedPDO::ATTR_TIMEOUT => 5and try/catch with generic errorserver/php/sandbox/index.php— addedDIRECTORY_SEPARATORtostr_starts_withpath traversal check.github/workflows/deploy-docker.yml— branch passed viaDEPLOY_BRANCHenv var (quoted) instead of inline${{ }}interpolation; added docker prune.github/workflows/deploy-php.yml— SSH job's "Load PHP_ENV" and both jobs' "Create server config" now read fromenv:block; configc()function escapes values and skips empty secrets; FTP/SSH deploy triggers added todeploy-to-phpbranchscripts/utils.js— fixedhostNameto use correct||fallback instead ofString()wrappingserver/php/README.md— corrected workflow filename todeploy-php.yml.github/workflows/deploy-ftp.yml— addeddeploy-to-phppush trigger
⚠️ FTP job "Load PHP_ENV" step has duplicate const env — SyntaxError at runtime
The deploy-ftp job's "Load PHP_ENV configuration" step (.github/workflows/deploy-php.yml) now declares env twice inside the same node -e script — once from the env-block-safe read (added by this commit) and once from the old inline interpolation (not removed). Redeclaring const in the same scope throws SyntaxError: Identifier 'env' has already been declared in Node.js, which would crash the step before any env vars are written to GITHUB_ENV, breaking FTP deployment.
The SSH job's equivalent step was fixed correctly (old line replaced with new line), but the FTP job's old line was left in place.
Technical details
# Duplicate const env in FTP "Load PHP_ENV"
## Affected sites
- `.github/workflows/deploy-php.yml:64-66` — two `const env = ...` declarations back-to-back, the second (`\`${{ vars.PHP_ENV }}\``) shadows and errors on the first
## Required outcome
- Remove the old inline-interpolated `const env = \`${{ vars.PHP_ENV }}\`.replace(/\r/g, '');` line, keeping only the safe `const env = (process.env.PHP_ENV_RAW || '').replace(/\r/g, '');` — mirroring the SSH job's correct fixDeepSeek Pro (free via Pullfrog for OSS) (Kimi K2 not used — the program covers this model; add its provider key to run your pick) ｜ 𝕏
There was a problem hiding this comment.
Important
The "Create server config" step backtick escaping was removed, exposing ${{ vars.PHP_ENV }} to shell command substitution. The previous review's duplicate const env issue in "Load PHP_ENV" is still present.
Reviewed changes — 1 commit (68534fd) that changed only deploy-php.yml "Create server config" steps: removed backtick escaping from the const phpEnv template literal and changed the esc function's inner .replace() from double quotes to a template literal.
.github/workflows/deploy-php.yml— unescaped backticks in both "Create server config" steps
⚠️ Unescaped backticks expose ${{ vars.PHP_ENV }} to shell command substitution
Both the FTP and SSH "Create server config" steps now use unescaped backticks around ${{ vars.PHP_ENV }} inside a node -e "..." double-quoted shell string. The shell interprets unescaped backticks as command substitution delimiters, meaning the expanded multi-line PHP_ENV content will be executed as shell commands rather than passed as a JavaScript template literal. This can produce incorrect phpEnv values (empty/corrupted) or execute arbitrary commands from the Actions variable content.
Technical details
# Shell command substitution via unescaped backticks
## Affected sites
- `.github/workflows/deploy-php.yml:95` — FTP "Create server config" step
- `.github/workflows/deploy-php.yml:205` — SSH "Create server config" step
## Required outcome
- Escape the backticks so the shell treats them as literal characters, not command substitution delimiters: `\`${{ vars.PHP_ENV }}\``ℹ️ Nitpicks
.github/workflows/deploy-php.yml:64-66— the FTP "Load PHP_ENV" step still has a duplicateconst envdeclaration (carried from previous commit). Theconst env = \${{ vars.PHP_ENV }}`.replace(...)on line 66 will throwSyntaxError: Identifier 'env' has already been declaredbecause line 64 already declaresconst env`. Remove line 66 and use only the env-block-safe read from line 64 — matching the SSH job's correct pattern.
DeepSeek Pro (free via Pullfrog for OSS) (Kimi K2 not used — the program covers this model; add its provider key to run your pick) ｜ 𝕏
There was a problem hiding this comment.
✅ No new issues found — the unescaped backtick regression from 68534fd has been reverted.
Reviewed changes — 1 commit (d527349) that restores backtick escaping on the const phpEnv template literal in both "Create server config" steps and re-escapes the esc function's inner template literal, reverting the shell command substitution exposure introduced in 68534fd.
.github/workflows/deploy-php.yml— restored ``` escaping on template literals in both "Create server config" steps
DeepSeek Pro (free via Pullfrog for OSS) (Kimi K2 not used — the program covers this model; add its provider key to run your pick) ｜ 𝕏
There was a problem hiding this comment.
✅ No new issues found.
Reviewed changes — this incremental delta fixes the last remaining open issue from prior reviews: the duplicate const env declaration in the "Load PHP_ENV" step that was causing a SyntaxError in the FTP deploy job.
- Removed duplicate
const envin both deploy jobs — the FTP "Load PHP_ENV" step previously had aconst envfromprocess.env.PHP_ENV_RAWimmediately shadowed byconst env = \${{ vars.PHP_ENV }}`` on the next line. The SSH job had been fixed earlier; the FTP job now matches. - Added
SELF_HOSTED=trueto GITHUB_ENV before build — ensuresnpm run build:appsees the correct self-hosted flag in both FTP and SSH jobs.
DeepSeek Pro (free via Pullfrog for OSS) (Kimi K2 not used — the program covers this model; add its provider key to run your pick) ｜ 𝕏
|
There was a problem hiding this comment.
✅ No new issues found. Previous review feedback addressed cleanly.
Reviewed changes — one clean-up commit with targeted improvements to the PHP server code and deploy workflow:
- Added
SELF_HOSTEDto server config generation — the deploy workflow now writes'SELF_HOSTED' => 'true'to the generated PHPconfig.php, consistent with theSELF_HOSTED=truealready added to the build environment in the prior commit. - Removed
LOWER()from share ID primary-key lookups —share.phpandimportProjectinutils.phpnow use exactid = ?comparisons instead ofLOWER(id) = LOWER(?). Generated IDs use only lowercase characters, so the indexed primary-key lookups are safe. - Added
CF-IPCountryto analytics log — server-side analytics now include the CloudflareCF-IPCountryheader for geographic attribution. - License and documentation touch-ups — corrected
lzstring.phplicense attribution, added explicit license URL, and updated.htaccesscomment.
ℹ️ Nitpicks
- The push trigger in
.github/workflows/deploy-php.ymlincludesdeploy-to-phpalongsidemain. This looks like a development convenience — the trigger should be removed before merging.
DeepSeek Pro (free via Pullfrog for OSS) (Kimi K2 not used — the program covers this model; add its provider key to run your pick) ｜ 𝕏
2 participants
Summary by CodeRabbit