GitHub Copilot In The IDE And Website

Review

The specific defect: == is a loose comparison, and comparing two strings with == is usually fine — until one of them can look like a number. PHP's loose comparison has historically had surprising numeric-string coercion behavior ("0e12345" == "0e67890" is true, because both are treated as 0 in scientific notation). If the token generation ever produces a string that looks like "0e..." followed by digits — which is exactly the kind of value a hex or base-encoded random token can accidentally produce — an attacker who can get the server to compare against a token starting with "0e" followed by digits can potentially match without knowing the real token. Separately, and just as importantly: this comparison is not constant-time. String comparison in PHP short-circuits on the first differing byte, so the time it takes to reject a wrong guess leaks information about how many leading characters were correct — a timing side-channel that, given enough attempts, can help an attacker guess the token faster than brute force alone would allow.

Why the manual test doesn't catch it: typing the correct token and confirming true only exercises the true-positive path. Both defects — the type-juggling edge case and the timing side-channel — only manifest with an incorrect token under specific conditions (a magic-hash-shaped guess, or a timing measurement across many attempts). A suggestion that "looks right" and passes the one test a developer intuitively reaches for is exactly the failure mode this lesson's article describes: plausible output that's wrong in a way that's easy to miss because the happy path works.

The fix: use hash_equals(), which PHP provides specifically for this purpose — a constant-time string comparison that also does not perform loose type coercion.

<?php

declare(strict_types=1);

function isValidResetToken(string $submittedToken, string $storedToken): bool
{
    return hash_equals($storedToken, $submittedToken);
}

var_dump(isValidResetToken('abc123', 'abc123'));
var_dump(isValidResetToken('wrong', 'abc123'));
// Prints:
// bool(true)
// bool(false)

hash_equals() takes the known/stored value first and the user-supplied value second — the fix doesn't just swap the operator, it uses the function designed for exactly this comparison, which is the durable lesson: security-sensitive comparisons (tokens, password hashes, HMACs) should almost never use == or === directly, because "are these two strings equal" and "are these two strings equal in a way that's safe against timing attacks and type coercion" are different questions.

What a reviewer should have flagged: any comparison involving a security token, secret, or hash should be an automatic pause point in review — "is this comparison safe against timing attacks and type juggling?" — regardless of how confidently the surrounding code reads or whether it came from Copilot, a search result, or a human teammate. This is precisely the class of code the article calls out as one where Copilot's plausible-looking output is least trustworthy, and where review scrutiny should go up, not down.

Diagnosis

First three pieces of evidence:

1. The error message itself — Call to undefined function array_get_or_warn() is unambiguous: this function does not exist anywhere in the loaded code (PHP core, installed extensions, or the project's own autoloaded classes/functions). That immediately rules out "it's defined somewhere I haven't found" and confirms this is a nonexistent-API problem, not a namespacing or autoload issue.
2. The PHP manual / a function_exists() check — confirming there is no array_get_or_warn in PHP's function list at any supported version rules out "maybe it's from a newer PHP version than we're running."
3. The pull request and its review — checking whether tests were run locally before merge, and what the reviewer actually looked at, tells you where the process broke: did the author run the code, or trust the suggestion on sight? Did the reviewer read for logic, or skim for style? This is the evidence that fixes the process, not just this line.

The false assumption: both the author and the reviewer assumed that a function name matching PHP's naming conventions (array_*, snake_case, parameter order that looks idiomatic) is evidence the function exists. It's an easy assumption to make precisely because it's usually a reliable signal for hand-written or copy-pasted code — a human copying from documentation or an IDE's autocomplete rarely invents a plausible-sounding nonexistent function, because autocomplete and documentation both constrain them to real APIs. Copilot has no such constraint: it generates the shape of a plausible PHP standard-library call because that shape is statistically common in its training data, independent of whether that specific function exists. The naming convention that would normally be a trust signal is, for AI-generated code, no signal at all.

Reproducing before production: the fastest catch is simply running the code locally, or letting CI run it — php -l (lint) would not have caught this, since array_get_or_warn(...) is syntactically valid; but actually executing the code path (a unit test that calls whatever function reads this config value) would have failed immediately with the same fatal error, in a local run or CI job instead of a production deploy. This is the general lesson: linting checks syntax, tests check behavior, and a hallucinated function name is a behavior problem that only running the code surfaces.

Durable fix — this line: replace with real PHP, which for "get with fallback, warn if missing" is a few lines of actual standard-library calls:

<?php

declare(strict_types=1);

function configValueOrWarn(array $config, string $key, mixed $default): mixed
{
    if (!array_key_exists($key, $config)) {
        trigger_error("Missing config key: {$key}, using default.", E_USER_WARNING);
        return $default;
    }
    return $config[$key];
}

$timeout = configValueOrWarn(['timeout' => 30], 'timeout', 60);
echo $timeout;
// Prints:
// 30

Durable fix — the process: the specific rule worth adopting is that any suggested function or method call the author or reviewer hasn't personally used before gets a one-second verification — function_exists(), a manual search, or just running the code — before merge. This costs almost nothing when the function is real, and it's the only thing that would have caught this before the deploy. Pairing that with "run the code, don't just read it" as a stated PR expectation addresses the root cause: this shipped because nobody actually executed the line between accepting the suggestion and merging the PR.

1. Content exclusions — enforced, not requested. Configure organization-level content exclusions for: any directory holding credentials or secrets even in example/template form (.env*, secrets/), and any vendored code under a license incompatible with the project's own. This is set at the org/repository level in GitHub's Copilot settings, not left as a per-developer reminder — a policy that depends on everyone remembering not to paste secrets into a chat prompt will fail exactly when someone's in a hurry.

2. Extra review scrutiny for security-sensitive categories, not for everything. Ordinary PR review applies to all AI-assisted code. For a specific, named list — authentication, authorization/access-control checks, cryptography and hashing, payment handling, and anything comparing secrets/tokens — the reviewer must explicitly confirm they read the logic line-by-line rather than skimming, and this is called out in the PR template as a checkbox for those categories only. Applying maximum scrutiny to every single line of every PR isn't practical and trains reviewers to skim everything; concentrating it on the categories where a plausible-looking mistake is most costly is what makes the policy sustainable.

3. Verify suggested APIs before merge. Any PR introducing a call to a function, method, or configuration option the author hasn't personally used before must include either a link to the actual documentation confirming it exists and behaves as expected, or a passing test that exercises it. This is the direct fix for the hallucinated-API failure mode — cheap to do, and it's a rule that's checkable in review ("where's the doc link or test for this call") rather than relying on the author's memory of whether they checked.

4. Copilot's PR review comments are input, not approval. Copilot-generated review comments are visible to human reviewers the same as any other comment, but they do not count toward required approvals, and a human reviewer is expected to form their own judgment rather than defer to or rubber-stamp past a Copilot comment. If Copilot flags something and a human reviewer disagrees, the human's call stands, documented with a one-line reason in the PR if the disagreement is non-obvious.

5. What this policy deliberately does not do: it does not require disclosing which lines were AI-suggested (unenforceable and not clearly useful — the review standard is the same either way), it does not ban Copilot from any part of the codebase outright, and it does not mandate a separate AI-specific review step for routine, low-risk changes (a config typo fix, a test scaffold). Keeping the policy to a handful of concrete, checkable rules — content exclusion enforced at the platform level, extra scrutiny on a named list of categories, a verification requirement for unfamiliar APIs, and clear precedence for human review over Copilot's own comments — is what makes it something the team will actually follow instead of a document nobody reads after the first week.