Add Security rules for Appwrite PHP conventions by abnegate · Pull Request #14062 · appwrite/appwrite

abnegate · 2026-10-02T02:52:16Z

What does this PR do?

Adds Security rules under .semgrep/ for Appwrite PHP coding conventions and wires them into CI. Rules are written as general pattern classes with allowlists, not lists of files or collections. CI changes only; no product code changes.

New ERROR findings fail Checks / Rules (and the scheduled Scan Rules job).
New WARNING findings do not fail the job. They are posted as one sticky PR comment () that is updated in place. Findings are grouped by rule. Each one has:
- a file:line link to the scanned commit;
- where it is: method + path + scope for routes, the hook and its groups, Class::method(), or the init resource;
- a short What's wrong and How to fix built from the matched code.
Baselined findings (.semgrep/baseline.json) neither fail the job nor appear in detail on the comment. The comment shows them only as per-rule counts in a collapsed block. They are still written to SARIF.

Baseline. Merging this PR does not change the result for other PRs. .semgrep/baseline.json records the 33 findings on the current tree, which is main merged into this branch. Semgrep OSS only supports a commit-diff baseline, so .semgrep/baseline.js filters the JSON scan itself:

check prints totals, lists new findings and stale entries, and exits 1 on any new ERROR.
update regenerates the file.

Entries are keyed on rule id, path, and the whitespace-collapsed matched text, so line shifts elsewhere in a file do not resurface them. The regeneration steps are in .semgrep/README.md.

ERROR classes (23). These have 0 findings on the current tree.

Area	Rules
Route declarations	`route-without-scope` (every `/v1` route declares a scope), `route-without-api-group` (non-public routes run the shared `api` hooks), `redirect-param-without-validator`
Authorization	`skip-ungated-load`, `global-authorization-disable`, `related-permissions-helper`
Credential responses	`account-token-secret-scope-gate` (now covers module `action()` methods and `Key` in any position), `show-sensitive-outside-payload`, `insecure-cookie-flags`
Guest-reachable input	`guest-url-without-publicurl` (any URL-like param name, `URL()` or `Text()`), `unbounded-map-to-outbound` (any `Assoc` param name; a class-level `ALLOWED_*` constant counts as filtered), `header-blocklist-filter`
Request metadata	`client-ip-header` (all forwarding headers and accessors, including host and proto)
Secrets and randomness	`default-secret-placeholder` (a class of placeholder literals), `insecure-random`
Outbound clients	`tls-verification-disabled`
Request-serving code	`unsafe-dynamic-code`, `shell-exec-in-handler`, `superglobal-in-handler`, `debug-output-in-handler`, `raw-sql-interpolation`, `xml-external-entities`, `request-path-to-filesystem` (taint from request getters or `string` action params)

WARNING classes (5). These have 33 findings on the current tree, all in the baseline. A class can be promoted to ERROR once its entries are cleared.

Rule	Findings
`guest-write-without-abuse-limit`	17
`permissive-write-permission`	8
`secret-compare-timing`	5
`weak-secret-env-default`	2
`outbound-follow-redirects`	1

Notes for rule authors (also in .semgrep/README.md):

Builtins need both spellings, for example unserialize(...) and \unserialize(...).
Route-level conditions match the whole method chain. Positive conditions go in metavariable-regex so that negative conditions can still see the full chain.
New rules add an entry to explainers in .github/workflows/semgrep-comment.js.

Test Plan

semgrep test .semgrep passes 28/28. Every rule has a sibling .php fixture with ruleid and ok cases. semgrep --validate reports 28 rules and 0 errors with baseline.js / baseline.json present.
bash .semgrep/prove.sh:
- Runs an ERROR-only scan (no baseline) on one known-bad snippet per ERROR rule and asserts that the scan fails with that rule id.
- Asserts that a file of house patterns produces no ERROR findings. The patterns include gated skips, PublicURL, redirectValidator, ALLOWED_HEADERS, getIP(), random_bytes, bound SQL, basename, httponly cookies, and event-payload showSensitive. A second clean file mirrors EncryptionKey (a PLACEHOLDER constant, comparisons against it, and a Console::warning naming it).
CI-equivalent scan on src/Appwrite app/controllers app/init:
- Without a baseline: 33 findings (0 ERROR).
- With the baseline: baseline.js check reports 33 baselined, 0 new ERROR, 0 new WARNING, and exits 0.
Baseline behaviour:
- Inserting lines at the top of a baselined file keeps its finding matched.
- Adding Permission::write(Role::any()) next to baselined lines gives 2 new WARNINGs (exit 0).
- An injected eval($request->getParam(...)) file gives 1 new ERROR (exit 1).
Comment rendering:
- The current tree renders the all-clear plus a collapsed per-rule summary of the 33 baselined findings.
- With a partial baseline, only the unbaselined findings are listed in detail, each with a link, location, What's wrong, and How to fix.
- Without a baseline, the full 33 findings render to about 21k characters.
Formatter stress test: the 128 fixture findings across all 28 rules each get an explanation. The comment stops at 59.9k characters with an "omitted" note.

Related PRs and Issues

None.

Checklist

Have you read the Contributing Guidelines on issues?
If the PR includes a change to an API's metadata (desc, label, params, etc.), does it also include updated API specs and example docs? (N/A: no API changes.)

Wire a Checks / Rules job into PR CI and the scheduled security scan so common skip, token-response, outbound-map, header-filter, and nested permission patterns fail the build instead of waiting on review. Co-authored-by: Jake Barnby <abnegate@users.noreply.github.com>

github-advanced-security · 2026-10-02T02:53:01Z

You are seeing this message because GitHub Code Scanning has recently been set up for this repository, or this pull request contains the workflow file for the Code Scanning tool.

What Enabling Code Scanning Means:

The 'Security' tab will display more code scanning analysis results (e.g., for the default branch).
Depending on your configuration and choice of analysis tool, future pull requests will be annotated with code scanning analysis results.
You will be able to see the analysis results for the pull request's branch on this overview once the scans have completed and the checks have passed.

For more information about GitHub Code Scanning, check out the documentation.

Expand skip and token-response matching, drop screenshots path excludes in favor of WARNING review signal, and add client-IP and placeholder-secret checks. ERROR scan on current main stays clean. Co-authored-by: Jake Barnby <abnegate@users.noreply.github.com>

Replace the userdata skip list with an allowlisted skip+load class, cover token/JWT output, related-permission name families, guest ROLE_GUESTS scopes, and guest URL params without PublicURL. Leave known screenshots sites as WARNING instead of path excludes. Co-authored-by: Jake Barnby <abnegate@users.noreply.github.com>

github-actions · 2026-10-02T03:05:04Z

✨ Benchmark results

Comparing main (before) → cursor/semgrep-static-rules-eb3f (after).

Metric	Before	After	Change
🚀 Requests/sec	`196.4`	`199.99`	⚪ `+1.8%`
⏱️ Latency P50	`87.6 ms`	`86.09 ms`	⚪ `-1.7%`
⏱️ Latency P95	`210.04 ms`	`202.69 ms`	⚪ `-3.5%`

Per-scenario breakdown & investigation details

Metrics below reflect the current branch (after). Δ P95 compares against the base.

Scenario	P50 (ms)	P95 (ms)	Requests	RPS	Δ P95 (ms)
API total	`86.09`	`202.69`	`12,540`	`199.99`	`-7.34`
Account	`171.83`	`340.21`	`660`	`11.19`	`+7.51`
TablesDB	`83.25`	`155.52`	`6,820`	`111.33`	`-3.14`
Storage	`79.55`	`172.69`	`3,300`	`55.46`	`+4.8`
Functions	`124.14`	`245.39`	`1,760`	`30.2`	`-17.74`

Top API waits (after)

API request	Max wait (ms)
account.name.update	`501.93`
functions.variables.update	`404.96`
functions.create	`402.64`
account.prefs.update	`370.35`
tablesdb.rows.list	`360.6`

…on PRs. Drop the named-collection ERROR leftover. Ungated skip+load of any non-allowlisted collection now fails CI; fixtures and prove.sh show a known-bad snippet fails and a gated skip does not. Checks / Rules upserts one sticky PR comment for WARNING (and ERROR) findings. Co-authored-by: Jake Barnby <abnegate@users.noreply.github.com>

Rename `static- rules` to `static-rules`. GitHub job ids cannot contain spaces, which left Checks / Rules (and the rest of CI) unscheduled. Co-authored-by: Jake Barnby <abnegate@users.noreply.github.com>

github-actions · 2026-10-02T03:43:03Z

Security rules

No new WARNING or ERROR findings from security rules.

33 existing findings tracked in .semgrep/baseline.json

php.appwrite.guest-write-without-abuse-limit (17)
php.appwrite.outbound-follow-redirects (1)
php.appwrite.permissive-write-permission (8)
php.appwrite.secret-compare-timing (5)
php.appwrite.weak-secret-env-default (2)

Posted by Checks / Rules. Re-runs update this comment in place. Rule details and baseline: .semgrep/README.md.

Replace the three-column table (160-char mid-sentence clip) with a bullet list of file:line, rule id, and the complete rule message. Drop whole findings only when the comment would exceed GitHub's size limit. Co-authored-by: Jake Barnby <abnegate@users.noreply.github.com>

client-ip-header covers every forwarding header and accessor; the placeholder rule matches a literal class; guest URL and Assoc rules match any URL-like / map param on ROLE_GUESTS scopes and honour class ALLOWED_* allowlists; the token gate covers module action() methods and Key in any position; header-blocklist-filter is ERROR now that main is clean. Co-authored-by: Jake Barnby <abnegate@users.noreply.github.com>

Every /v1 route needs a scope label, non-public routes need the api hook group, redirect-target params need redirectValidator, and guest-scoped writes without an abuse limit are surfaced as WARNING. Co-authored-by: Jake Barnby <abnegate@users.noreply.github.com>

Dynamic code, process execution, disabled TLS verification, superglobals, request-wide authorization disable, showSensitive outside event payloads, non-cryptographic randomness, libxml entity loading, interpolated SQL, non-httponly cookies, debug output, and request-derived filesystem paths. Co-authored-by: Jake Barnby <abnegate@users.noreply.github.com>

Secret env defaults, non-constant-time secret comparisons, any/guests write permissions, and outbound clients that follow redirects. Each has live findings on main and is listed on the sticky PR comment. Co-authored-by: Jake Barnby <abnegate@users.noreply.github.com>

prove.sh runs the real ERROR scan on one known-bad snippet per ERROR rule, asserts the expected rule id, and checks a file of house patterns stays clean. README gains a full trigger matrix and rule-writing notes. Co-authored-by: Jake Barnby <abnegate@users.noreply.github.com>

EncryptionKey on main declares the public default as a PLACEHOLDER constant and compares against it; constant propagation made every use an ERROR. Comparisons, warning/exception messages and a PLACEHOLDER constant are now ok, while another class's PLACEHOLDER as a getEnv default is caught explicitly. Co-authored-by: Jake Barnby <abnegate@users.noreply.github.com>

…mment. The formatter reads each matched file from the checkout to resolve the HTTP method, path and scope (or hook, Class::method, init resource), then prints a per-rule What's wrong / How to fix built from the matched code, with a link to the scanned commit. Findings are grouped by rule so the full rule message appears once per group. Co-authored-by: Jake Barnby <abnegate@users.noreply.github.com>

…-rules-eb3f Co-authored-by: Jake Barnby <abnegate@users.noreply.github.com>

Co-authored-by: Jake Barnby <abnegate@users.noreply.github.com>

…nd Scan Rules. Co-authored-by: Jake Barnby <abnegate@users.noreply.github.com>

Co-authored-by: Jake Barnby <abnegate@users.noreply.github.com>

hansi-codes · 2026-10-02T08:47:18Z

🔵 Tier A · Mergeable after minor fixes

The implementation has minor false-negative and reporting issues in rule exemptions, baseline matching, and failed-scan comments.

Adds 28 custom Semgrep rules with PHP fixtures and scratch proofs for Appwrite security conventions. Integrates scans into PR and scheduled CI, with a checked-in findings baseline, SARIF uploads, and sticky PR comments for new findings.

Verdict	New comments	Fixed	Still open
💬 Commented	3	0	0

Note

Part of the diff was too large to review, so Hansi did not approve.

	Finding	Where
🟡	Do not treat realpath alone as a traversal sanitizer	`.semgrep/request-path-to-filesystem.yml:44`
🟡	Avoid posting an all-clear when the scan never produced JSON	`.github/workflows/semgrep-comment.js:31`
🟡	Include the matched route identity in baseline keys	`.semgrep/baseline.js:29`

Fix with agent prompt

### Issue 1
.semgrep/request-path-to-filesystem.yml:44-45
**Do not treat realpath alone as a traversal sanitizer**

`realpath()` resolves `..` and symlinks but does not restrict the result to the intended directory, so `file_get_contents(realpath($base . '/' . $request->getParam('path')))` escapes this rule even when it reads outside `$base`. Keep the value tainted until a directory-containment check is enforced, and add a fixture for unchecked `realpath()`.

### Issue 2
.github/workflows/semgrep-comment.js:31-33
**Avoid posting an all-clear when the scan never produced JSON**

The workflow runs this comment step with `always()`, so an installation, fixture, proof, or scan failure can reach this missing-file branch. Returning an empty findings list then overwrites the sticky comment with “No new WARNING or ERROR findings”; skip the update or explicitly report that the scan did not complete instead.

### Issue 3
.semgrep/baseline.js:29-33
**Include the matched route identity in baseline keys**

For multiline route findings, this stops after `$this ->setHttpMethod(...)`, before `setHttpPath()`; the transaction entries in `baseline.json` already demonstrate this. Replacing a baselined route with a different unrate-limited route at the same path and HTTP method is therefore silently baselined, contrary to the documented behavior that editing matched code creates a new finding; include the matched span or at least the route path in the key.

---

For each issue above, determine whether it is valid and should be fixed. If so, fix it directly.

📂 Walkthrough · 8

File	Change
`.github/workflows/ci.yml`	Adds fixture validation, scratch proofs, security scans, baseline enforcement, SARIF uploads, and PR comments.
`.github/workflows/security-scan.yml`	Adds the equivalent security-rule scan to the scheduled workflow.
`.github/workflows/semgrep-comment.js`	Formats findings with source context and per-rule explanations, then updates a sticky PR comment.
`.gitignore`	Ignores the temporary directory used by scratch proofs.
`.semgrep/README.md`	Documents rules, local scans, baseline maintenance, fixtures, and PR reporting.
`.semgrep/baseline.js, .semgrep/baseline.json`	Adds baseline generation and filtering with 33 existing WARNING findings.
`.semgrep/.yml, .semgrep/.php`	Adds 28 security rules and matching positive and negative PHP fixtures.
`.semgrep/prove.sh`	Checks that known-bad snippets trigger ERROR rules and house patterns remain clean.

🔇 Filtered out · 1

Findings Hansi considered but did not post.

Finding	Why
Restrict skip exemptions to actual privileged conditions	Not on a changed line

_{Reviewed de1963c · Details · Comment @hansi-codes review to re-run, or mention @hansi-codes with a question.}

hansi-codes

🔵 Tier A · See the inline comments. Summary

hansi-codes · 2026-10-02T08:47:21Z

+      - pattern: realpath(...)
+      - pattern: \realpath(...)


Do not treat realpath alone as a traversal sanitizer

realpath() resolves .. and symlinks but does not restrict the result to the intended directory, so file_get_contents(realpath($base . '/' . $request->getParam('path'))) escapes this rule even when it reads outside $base. Keep the value tainted until a directory-containment check is enforced, and add a fixture for unchecked realpath().

Prompt To Fix With AI

This is a comment left during a code review. Path: .semgrep/request-path-to-filesystem.yml Line: 44-45 Comment: **Do not treat realpath alone as a traversal sanitizer** `realpath()` resolves `..` and symlinks but does not restrict the result to the intended directory, so `file_get_contents(realpath($base . '/' . $request->getParam('path')))` escapes this rule even when it reads outside `$base`. Keep the value tainted until a directory-containment check is enforced, and add a fixture for unchecked `realpath()`. --- For each issue above, determine whether it is valid and should be fixed. If so, fix it directly.

_{🟡 Minor · bug · Reply if this doesn't apply.}

hansi-codes · 2026-10-02T08:47:21Z

+    if (!fs.existsSync(path)) {
+        core?.warning(`Semgrep JSON not found at ${path}`);
+        return [];


Avoid posting an all-clear when the scan never produced JSON

The workflow runs this comment step with always(), so an installation, fixture, proof, or scan failure can reach this missing-file branch. Returning an empty findings list then overwrites the sticky comment with “No new WARNING or ERROR findings”; skip the update or explicitly report that the scan did not complete instead.

Prompt To Fix With AI

This is a comment left during a code review. Path: .github/workflows/semgrep-comment.js Line: 31-33 Comment: **Avoid posting an all-clear when the scan never produced JSON** The workflow runs this comment step with `always()`, so an installation, fixture, proof, or scan failure can reach this missing-file branch. Returning an empty findings list then overwrites the sticky comment with “No new WARNING or ERROR findings”; skip the update or explicitly report that the scan did not complete instead. --- For each issue above, determine whether it is valid and should be fixed. If so, fix it directly.

_{🟡 Minor · error-handling · Reply if this doesn't apply.}

hansi-codes · 2026-10-02T08:47:21Z

+    let text = (lines[start - 1] || '').slice(Math.max(0, (result.start?.col || 1) - 1));
+    for (let line = start + 1; line <= Math.min(end, start + TEXT_LINES - 1) && text.trim().length < TEXT_MIN; line++) {
+        text += ' ' + (lines[line - 1] || '');
+    }
+    return text.replace(/\s+/g, ' ').trim();


Include the matched route identity in baseline keys

For multiline route findings, this stops after $this ->setHttpMethod(...), before setHttpPath(); the transaction entries in baseline.json already demonstrate this. Replacing a baselined route with a different unrate-limited route at the same path and HTTP method is therefore silently baselined, contrary to the documented behavior that editing matched code creates a new finding; include the matched span or at least the route path in the key.

Prompt To Fix With AI

This is a comment left during a code review. Path: .semgrep/baseline.js Line: 29-33 Comment: **Include the matched route identity in baseline keys** For multiline route findings, this stops after `$this ->setHttpMethod(...)`, before `setHttpPath()`; the transaction entries in `baseline.json` already demonstrate this. Replacing a baselined route with a different unrate-limited route at the same path and HTTP method is therefore silently baselined, contrary to the documented behavior that editing matched code creates a new finding; include the matched span or at least the route path in the key. --- For each issue above, determine whether it is valid and should be fixed. If so, fix it directly.

_{🟡 Minor · bug · Reply if this doesn't apply.}

cursoragent and others added 2 commits October 2, 2026 03:00

cursoragent and others added 3 commits October 2, 2026 03:33

Merge branch 'main' into cursor/semgrep-static-rules-eb3f

b4f0bfd

Fix invalid CI job id so pull_request workflows schedule.

8d58677

Rename `static- rules` to `static-rules`. GitHub job ids cannot contain spaces, which left Checks / Rules (and the rest of CI) unscheduled. Co-authored-by: Jake Barnby <abnegate@users.noreply.github.com>

cursoragent and others added 6 commits October 2, 2026 04:03

cursor Bot changed the title ~~Add custom Semgrep rules for authorization and data-handling review~~ Add in-repo Semgrep rule classes for Appwrite PHP conventions Oct 2, 2026

cursoragent and others added 7 commits October 2, 2026 05:45

Merge remote-tracking branch 'origin/main' into cursor/semgrep-static…

cc7042f

…-rules-eb3f Co-authored-by: Jake Barnby <abnegate@users.noreply.github.com>

Add a baseline of current security rule findings.

4f959d4

Co-authored-by: Jake Barnby <abnegate@users.noreply.github.com>

List only findings outside the baseline on the security rules comment.

0ef96d5

Co-authored-by: Jake Barnby <abnegate@users.noreply.github.com>

Check security rule findings against the baseline in Checks / Rules a…

23eb3fe

…nd Scan Rules. Co-authored-by: Jake Barnby <abnegate@users.noreply.github.com>

Rename to Security rules in the rules README and document the baseline.

de1963c

Co-authored-by: Jake Barnby <abnegate@users.noreply.github.com>

cursor Bot changed the title ~~Add in-repo Semgrep rule classes for Appwrite PHP conventions~~ Add Security rules for Appwrite PHP conventions Oct 2, 2026

abnegate marked this pull request as ready for review October 2, 2026 08:45

hansi-codes Bot reviewed Oct 2, 2026

View reviewed changes

Navigation Menu

Add Security rules for Appwrite PHP conventions - #14062

Add Security rules for Appwrite PHP conventions#14062
abnegate wants to merge 19 commits into
mainfrom
cursor/semgrep-static-rules-eb3f

abnegate commented Oct 2, 2026 •

edited by cursor Bot

Loading

Uh oh!

github-advanced-security AI commented Oct 2, 2026

Uh oh!

github-actions Bot commented Oct 2, 2026 •

edited

Loading

Uh oh!

github-actions Bot commented Oct 2, 2026 •

edited

Loading

Uh oh!

hansi-codes Bot commented Oct 2, 2026

Uh oh!

hansi-codes Bot left a comment

Uh oh!

hansi-codes Bot Oct 2, 2026

Uh oh!

hansi-codes Bot Oct 2, 2026

Uh oh!

hansi-codes Bot Oct 2, 2026

Uh oh!

This branch has not been deployed

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

Conversation

abnegate commented Oct 2, 2026 • edited by cursor Bot Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

What does this PR do?

Test Plan

Related PRs and Issues

Checklist

Uh oh!

github-advanced-security AI commented Oct 2, 2026

What Enabling Code Scanning Means:

Uh oh!

github-actions Bot commented Oct 2, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

✨ Benchmark results

Uh oh!

github-actions Bot commented Oct 2, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Security rules

Uh oh!

hansi-codes Bot commented Oct 2, 2026

🔵 Tier A · Mergeable after minor fixes

Uh oh!

hansi-codes Bot left a comment

Choose a reason for hiding this comment

Uh oh!

hansi-codes Bot Oct 2, 2026

Choose a reason for hiding this comment

Uh oh!

hansi-codes Bot Oct 2, 2026

Choose a reason for hiding this comment

Uh oh!

hansi-codes Bot Oct 2, 2026

Choose a reason for hiding this comment

Uh oh!

This branch has not been deployed

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

abnegate commented Oct 2, 2026 •

edited by cursor Bot

Loading

github-actions Bot commented Oct 2, 2026 •

edited

Loading

github-actions Bot commented Oct 2, 2026 •

edited

Loading