Conversation
There was a problem hiding this comment.
Pull request overview
Adds a new PHP language security rules document under rules/languages/php/, following the repository’s rule format (Level/When/Do/Don’t/Why/Refs) and including a Quick Reference section.
Changes:
- Added PHP security guidance covering database, filesystem, sessions, error handling, input handling, code execution, cryptography, and configuration hardening.
- Added a PHP “Quick Reference” table and version history entry.
Comments suppressed due to low confidence (5)
rules/languages/php/CLAUDE.md:76
- This example uses
str_starts_with, which is only available in PHP 8+. If these rules are intended to apply to PHP 7.x as well, add a brief version note or use a compatible prefix check approach so the snippet is broadly usable.
if ($fullPath === false || !str_starts_with($fullPath, $baseDir . DIRECTORY_SEPARATOR)) {
throw new RuntimeException('Path traversal attempt detected.');
}
rules/languages/php/CLAUDE.md:327
- The “Safe” command execution example references
$safeFilePathand$validatedFilenamebut neither is defined in the snippet. Since the checklist says examples are copy/paste ready, define these variables (or inline the validation step) so the example is self-contained.
$op = $_GET['op'] ?? '';
if (!array_key_exists($op, $allowedOperations)) {
throw new InvalidArgumentException('Unknown operation.');
}
$fn = $allowedOperations[$op];
$result = $fn($safeFilePath);
// Safe: If a shell command is unavoidable, use escapeshellarg() on every argument
$safeArg = escapeshellarg($validatedFilename);
$output = shell_exec("wc -l $safeArg");
**rules/languages/php/CLAUDE.md:163**
* `ini_set('session.cookie_secure', 1)` is correct for HTTPS, but if applied unconditionally it will break sessions over HTTP (common in local/dev or misconfigured environments). Consider adjusting the guidance to set it conditionally when the request is HTTPS / behind a TLS-terminating proxy, or clarify the assumption that the app is always served over HTTPS.
ini_set('session.cookie_httponly', 1); // Block JavaScript access to cookie
ini_set('session.cookie_secure', 1); // HTTPS only
ini_set('session.cookie_samesite', 'Strict');
ini_set('session.sid_length', 32); // Manual recommends 32 chars minimum
**rules/languages/php/CLAUDE.md:452**
* The Quick Reference table has an extra leading `|` (`|| Rule | Level | CWE |`), which prevents correct Markdown table rendering. Update it to the same table format used in other language rule files (single leading/trailing pipes).
| Rule | Level | CWE |
|---|---|---|
| Parameterized queries | strict | CWE-89 |
| Prevent path traversal | strict | CWE-22 |
**rules/languages/php/CLAUDE.md:78**
* The `safeReadFile` helper can silently disable its path traversal protection if `$baseDir` does not resolve via `realpath()`. In that case `$baseDir` becomes `false`, the prefix check degrades to `str_starts_with($fullPath, '/')`, and any absolute path (e.g., `/etc/passwd`) will pass the check and be read, breaking the intended directory restriction. To keep this pattern safe, treat a `false` result from `realpath($baseDir)` as an error and fail closed (e.g., throw) before building or validating `$fullPath`.
function safeReadFile(string $filename, string $baseDir = '/app/data'): string {
$baseDir = realpath($baseDir);
$fullPath = realpath($baseDir . DIRECTORY_SEPARATOR . $filename);
if ($fullPath === false || !str_starts_with($fullPath, $baseDir . DIRECTORY_SEPARATOR)) {
throw new RuntimeException('Path traversal attempt detected.');
}
return file_get_contents($fullPath);
</details>
---
💡 <a href="/TikiTribe/claude-secure-coding-rules/new/main?filename=.github/instructions/*.instructions.md" class="Link--inTextBlock" target="_blank" rel="noopener noreferrer">Add Copilot custom instructions</a> for smarter, more guided reviews. <a href="https://docs.github.com/en/copilot/customizing-copilot/adding-repository-custom-instructions-for-github-copilot" class="Link--inTextBlock" target="_blank" rel="noopener noreferrer">Learn how to get started</a>.
- Fail closed in safeReadFile() when realpath() returns false for $baseDir - Make session.cookie_secure conditional on HTTPS availability - Define $safeFilePath and $validatedFilename inline in code execution snippet - Move declare(strict_types=1) to its own snippet with file-position note - Add RandomException handling around random_bytes() calls - Add OWASP A01:2025 ref to Handle Null Bytes rule - Fix bare URL in Version History (MD034)
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 1 out of 1 changed files in this pull request and generated 1 comment.
Comments suppressed due to low confidence (8)
rules/languages/php/CLAUDE.md:55
- The OWASP reference here appears to point to Injection, but in
rules/_core/owasp-2025.mdA03:2025 is “Software Supply Chain Failures” and Injection is A05:2025. Please update this reference to the correct OWASP Top 10 2025 category to stay consistent with the core rules.
**Why**: SQL injection allows attackers to read, modify, or delete database data, and on some servers can escalate to OS-level command execution.
**Refs**: CWE-89, OWASP A03:2025
rules/languages/php/CLAUDE.md:247
- This rule is about production error display / configuration, but the OWASP reference is A05:2025 (Injection). In
rules/_core/owasp-2025.md, misconfiguration guidance lives under A02:2025. Please update the OWASP reference to the appropriate category.
**Why**: Error messages reveal server paths, database schemas, library versions, and variable names — all useful to an attacker profiling the system.
**Refs**: CWE-209, OWASP A05:2025
rules/languages/php/CLAUDE.md:358
- This OWASP reference uses A03:2025, but the core OWASP file defines A03 as “Software Supply Chain Failures.” For command/code injection concerns, the matching OWASP Top 10 2025 category in
rules/_core/owasp-2025.mdis A05:2025 (Injection). Please update the reference.
**Why**: Functions like `eval()`, `exec()`, and `include` with user-controlled input enable arbitrary code and command execution, potentially resulting in full server compromise.
**Refs**: CWE-94, CWE-78, CWE-98, OWASP A03:2025
rules/languages/php/CLAUDE.md:133
- This example uses
str_contains(), which requires PHP 8.0+. The surrounding docs call out PHP version requirements elsewhere (e.g.,str_starts_with), but not here. Please either note the PHP 8.0+ requirement or use a compatible alternative (e.g.,strpos($input, "\0") !== false).
// Safe: Reject strings containing null bytes before any file operation
function sanitizeFilename(string $input): string {
if (str_contains($input, "\0")) {
throw new InvalidArgumentException('Null byte detected in filename.');
}
return basename($input);
}
// Safe: PHP 5.3.4+ raises a warning, but explicit checks are still best practice
$filename = sanitizeFilename($_GET['file']);
rules/languages/php/CLAUDE.md:302
- The OWASP reference here points to A03:2025, but in
rules/_core/owasp-2025.mdA03 is “Software Supply Chain Failures.” For input validation/injection-related guidance, A05:2025 (Injection) is the matching category in the core rules. Please update the OWASP reference accordingly.
**Why**: Every value from the browser — including cookies and hidden fields — is attacker-controlled. Unvalidated input is the root cause of injection, XSS, and filesystem attacks.
**Refs**: CWE-20, CWE-79, OWASP A03:2025
rules/languages/php/CLAUDE.md:412
- This rule is about cryptographic failures (password hashing and randomness), but it references OWASP A02:2025 (Security Misconfiguration). In
rules/_core/owasp-2025.md, cryptography guidance is under A04:2025 (Cryptographic Failures). Please update the OWASP reference to match the core categorization.
**Why**: Weak hashing algorithms and predictable random number generators allow attackers to crack passwords and forge tokens offline.
**Refs**: CWE-327, CWE-328, CWE-330, OWASP A02:2025
rules/languages/php/CLAUDE.md:460
- This rule is configuration hardening / minimizing exposure, but it references OWASP A05:2025 (Injection). In
rules/_core/owasp-2025.md, configuration hardening guidance maps to A02:2025 (Security Misconfiguration). Please update the OWASP reference to the appropriate category.
**Why**: Exposing the PHP version, server configuration, and include paths gives attackers a detailed map to exploit known vulnerabilities and craft targeted attacks.
**Refs**: CWE-200, OWASP A05:2025
rules/languages/php/CLAUDE.md:88
- The comment says “Whitelist with basename() and ctype checks” but the example uses a regex and no
ctype_*checks. Please adjust the wording to match the actual validation approach, or add the referencedctypevalidation if that’s the intent.
// Safe: Whitelist with basename() and ctype checks
$filename = basename($_POST['filename']);
if (!preg_match('/^[a-zA-Z0-9_-]+\.(csv|txt)$/', $filename)) {
throw new InvalidArgumentException('Invalid filename.');
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
…dback) - Correct OWASP refs to match rules/_core/owasp-2025.md categorisation: - Parameterized Queries, Validate Input, Dangerous Functions: A03 -> A05 (Injection) - Disable Error Display, Minimize PHP Exposure: A05 -> A02 (Security Misconfiguration) - Password Hashing and Randomness: A02 -> A04 (Cryptographic Failures) - Rename safeReadFile() to safeValidatePath(); return validated path not file contents - Fix comment 'basename() and ctype checks' to match actual preg_match implementation - Add PHP 8.0+ version note and PHP 7.x alternative to str_starts_with in safeValidatePath() - Add PHP 8.0+ version note and PHP 7.x alternative to str_contains in sanitizeFilename()
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 1 out of 1 changed files in this pull request and generated no new comments.
Comments suppressed due to low confidence (6)
rules/languages/php/CLAUDE.md:478
- The Quick Reference markdown table has an extra empty column because each row starts with
||instead of|. This will render incorrectly in Markdown; update the table to use a single leading/trailing pipe per row (matching other language rule files).
| Rule | Level | CWE |
|------|-------|-----|
| Parameterized queries | strict | CWE-89 |
| Prevent path traversal | strict | CWE-22 |
| Handle null bytes in paths | strict | CWE-158 |
| Harden session configuration | strict | CWE-384 |
| Disable error display in production | strict | CWE-209 |
| Validate and escape user input | strict | CWE-20 |
| Avoid dangerous functions with user input | strict | CWE-94 |
| Secure password hashing and randomness | strict | CWE-327 |
| Minimize PHP exposure | warning | CWE-200 |
rules/languages/php/CLAUDE.md:84
safeValidatePath()usesrealpath()on the full path, which returnsfalsefor non-existent targets. That makes this example unsuitable for the "writing" case mentioned in the rule (e.g., validating a new upload path before creating the file). Either narrow the rule/example to existing files (read/delete) or adjust the example to validate the base directory and the parent directory without requiring the final file to already exist.
// Safe: Validate path and return it — does NOT read the file
// (requires PHP 8.0+ for str_starts_with; use substr($fullPath, 0, strlen($resolvedBase)) === $resolvedBase for PHP 7.x)
function safeValidatePath(string $filename, string $baseDir = '/app/data'): string {
$resolvedBase = realpath($baseDir);
if ($resolvedBase === false) {
throw new \RuntimeException('Base directory does not exist.');
}
$fullPath = realpath($resolvedBase . DIRECTORY_SEPARATOR . $filename);
if ($fullPath === false || !str_starts_with($fullPath, $resolvedBase . DIRECTORY_SEPARATOR)) {
throw new \RuntimeException('Path traversal attempt detected.');
}
return $fullPath; // return the validated path, not the file contents
}
rules/languages/php/CLAUDE.md:90
- Exceptions are referenced inconsistently:
\RuntimeExceptionis fully-qualified, butInvalidArgumentExceptionis not. In namespaced PHP code,InvalidArgumentExceptionwould resolve relative to the current namespace unless imported. Use fully-qualified\InvalidArgumentException(or add an explicituse) consistently in examples.
if (!preg_match('/^[a-zA-Z0-9_-]+\.(csv|txt)$/', $filename)) {
throw new InvalidArgumentException('Invalid filename.');
}
rules/languages/php/CLAUDE.md:391
- The
random_bytes()example catches\Random\RandomException, which only exists in newer PHP versions (PHP 8.2+). Since this doc already includes PHP version compatibility notes elsewhere, either add a version note here or catch\Throwable/\Exceptionso the snippet is copy-paste safe across supported PHP versions.
try {
$token = bin2hex(random_bytes(32)); // 64-char hex token
$apiKey = base64_encode(random_bytes(32));
} catch (\Random\RandomException $e) {
throw new \RuntimeException('Failed to generate secure random bytes.', 0, $e);
}
rules/languages/php/CLAUDE.md:384
- The password hashing snippet says "bcrypt by default" but explicitly uses
PASSWORD_BCRYPT. This is slightly misleading and also prevents algorithm upgrades. Prefer showingPASSWORD_DEFAULT+password_needs_rehash()guidance, or reword the comment to match the code (and explain why choosing a fixed algorithm is intentional, if it is).
// Safe: Password hashing with password_hash() (bcrypt by default)
$hash = password_hash($plaintext, PASSWORD_BCRYPT, ['cost' => 12]);
// Safe: Verification
if (password_verify($plaintext, $hash)) {
// authenticated
}
// Safe: Upgrade to Argon2id when available
$hash = password_hash($plaintext, PASSWORD_ARGON2ID);
rules/languages/php/CLAUDE.md:171
ini_set('session.cookie_samesite', 'Strict')requires PHP 7.3+ (the directive doesn't exist on older versions). Since other sections call out PHP-version dependencies, add a compatibility note or show an alternative (e.g., setting SameSite viasession_set_cookie_params()where available) so the snippet remains copy-paste ready.
ini_set('session.cookie_httponly', 1); // Block JavaScript access to cookie
ini_set('session.cookie_secure', isset($_SERVER['HTTPS']) ? 1 : 0); // HTTPS only when available
ini_set('session.cookie_samesite', 'Strict');
ini_set('session.sid_length', 32); // Manual recommends 32 chars minimum
ini_set('session.sid_bits_per_character', 5);
session_start();
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
- Add PHP to language lists, directory structures, and rule counts in CLAUDE.md, README.md, and docs/CONTRIBUTING.md - Narrow safeValidatePath() to existing files; add safeWritePath() for write targets where realpath() would return false - Fully qualify all InvalidArgumentException references with leading \ - Switch PASSWORD_BCRYPT to PASSWORD_DEFAULT with password_needs_rehash() - Catch \Exception instead of \Random\RandomException for PHP <8.2 compat - Add PHP version notes for session.cookie_samesite (7.3+) and Argon2id Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 4 out of 4 changed files in this pull request and generated no new comments.
Comments suppressed due to low confidence (1)
rules/languages/php/CLAUDE.md:104
- In
safeWritePath(), thestr_starts_with($fullPath, $resolvedBase . DIRECTORY_SEPARATOR)check is ineffective because$fullPathis constructed by prefixing$resolvedBasedirectly, so the condition will always be true. This can give a false sense of security—either remove the check or replace it with a validation that can actually fail (e.g., ensure$sanitizedcontains no path separators / stream wrapper patterns, or validate the resolved parent directory).
$fullPath = $resolvedBase . DIRECTORY_SEPARATOR . $sanitized;
if (!str_starts_with($fullPath, $resolvedBase . DIRECTORY_SEPARATOR)) {
throw new \RuntimeException('Path traversal attempt detected.');
}
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
The check always passed because $fullPath was constructed by directly prefixing $resolvedBase, giving a false sense of security. The actual traversal protection is provided by basename() and the regex allowlist, which are documented in a clarifying comment. Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 4 out of 4 changed files in this pull request and generated no new comments.
Comments suppressed due to low confidence (4)
rules/languages/php/CLAUDE.md:340
- In this "Do" example you recommend disabling
shell_execviadisable_functions, but a few lines later you useshell_exec()as the safe fallback. This is internally inconsistent and may confuse readers—either removeshell_execfrom the recommendeddisable_functionslist, or explicitly call out that the shell example only applies when that function is intentionally allowed/enabled.
// Safe: Use disable_functions in php.ini to block dangerous functions entirely
// disable_functions = exec,passthru,shell_exec,system,proc_open,popen,eval
rules/languages/php/CLAUDE.md:361
- This snippet validates a safe path into
$safePath, but the shell example then discards it and usesbasename($_GET['file'])instead. To keep the guidance coherent (and avoid implying thatbasename()is sufficient), reuse the already-validated$safePathwhen building the shell command argument.
// Validate the path — safeValidatePath() returns a safe path, not file contents
$safePath = safeValidatePath($_GET['file'] ?? '', '/app/data');
$fn = $allowedOperations[$op];
$result = $fn($safePath);
// Safe: If a shell command is unavoidable, use escapeshellarg() on every argument
$validatedFilename = basename($_GET['file'] ?? '');
$safeArg = escapeshellarg($validatedFilename);
$output = shell_exec("wc -l $safeArg");
docs/CONTRIBUTING.md:18
- The statement "This project provides 27 security rule sets" appears inconsistent with the repository scope described elsewhere (e.g., README mentions 100+ rule sets and the repo has additional categories like
rules/rag/,rules/containers/,rules/iac/,rules/cicd/). Consider rewording this to clarify you’re counting only core + language + backend/frontend sets, or update the count/categories to reflect the full repository.
This project provides **27 security rule sets** covering:
- **4 Core rule sets**: OWASP 2025, MCP Security, AI/ML Security, Agent Security
- **13 Language rules**: Python, JavaScript, TypeScript, Go, Rust, Java, C#, Ruby, R, C++, Julia, SQL, PHP
- **5 Backend frameworks**: FastAPI, Express, Django, Flask, NestJS
- **5 Frontend frameworks**: React, Next.js, Vue, Angular, Svelte
CLAUDE.md:93
- The "Total Rule Sets" count (38) and the category breakdown here excludes other rule-set directories that exist in the repo (e.g.,
rules/rag/,rules/containers/,rules/iac/,rules/cicd/). This makes the total potentially misleading and inconsistent with README’s "100+ Rule Sets"—please clarify what this table is counting or expand it to include the missing categories.
| Category | Count | Description |
|----------|-------|-------------|
| Core Rules | 4 | OWASP 2025, MCP Security, AI Security, Agent Security |
| Languages | 13 | Python, JavaScript, TypeScript, Go, Rust, Java, C#, Ruby, R, C++, Julia, SQL, PHP |
| Backend Frameworks | 5 | FastAPI, Express, Django, Flask, NestJS |
| AI/ML Frameworks | 11 | LangChain, CrewAI, AutoGen, Transformers, vLLM, Triton, TorchServe, Ray Serve, BentoML, MLflow, Modal |
| Frontend Frameworks | 5 | React, Next.js, Vue, Angular, Svelte |
| **Total Rule Sets** | **38** | Comprehensive security coverage |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
…t PR feedback) Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
Description
Adds PHP security rules to the language rules section.
Type of Change
Checklist
For Rule Changes
For All Changes
pytest tests/)Standards Coverage
Testing
pytest tests/structural/- format validationpytest tests/code_validation/- code syntaxRelated Issues
Additional Notes
My run of
pytest tests/code_validation/highlighted 7 errors but none of them were related to the PHP security rules file that I added.