Repository navigation
AFC-465: bring the Infisical loader in line with the fleet copy - #1763
Conversation
The loader here still decided whether it was the entry module by comparing two paths as text, so a path that resolved to another directory made the loader do nothing at all. The fleet copy compares the two as real paths, and that is the shape this repo now carries. The fleet's own loader test comes with it: it exercises the loader's command-line entry through a symlinked path, which is the shape a production Mac path has, because /var is a link to /private/var. The test is wired into both npm test and npm run test:coverage, and the coverage script is what this repo's CI runs. Refs AFC-465
Code Review Completed! 🔥The code review was successfully completed based on your current configurations. Kody Guide: Usage and ConfigurationInteracting with Kody
Current Kody ConfigurationReview OptionsThe following review options are enabled or disabled:
|
|
Source comment has a one-space sentence boundary The inconsistent boundary makes this comment diverge from the repository's plain-text source convention. Use two ASCII spaces after Kody rule violation: Sentence gap by surface: two ASCII spaces in files, U+00A0 plus a space in rendered copy and GitHub text Prompt for LLM |
|
Task description is insufficient for business logic validation. 🤔 Insufficient Task ContextI found a task linked to this PR, but it only contains minimal information (title only, no description or acceptance criteria). To perform a meaningful business rules validation, I need more details. 🔍 What I need to validate:
💡 How to improve the task context:
|
![kody-ai[bot]](https://avatars.githubusercontent.com/in/413034?s=60&v=4){kind=small}
| const token = await loginUniversalAuth({ siteUrl, clientId, clientSecret, fetchImpl }); | ||
| // The access token rides in an Authorization header below and is never | ||
| // printed; mask it anyway so no future error path can surface it. | ||
| maskValue(token, log); | ||
| return await listSecretsRaw({ siteUrl, token, projectId, environment, secretPath, fetchImpl }); |
There was a problem hiding this comment.
Access token no longer masked, so an error path can leak it to the public log
If loginUniversalAuth() returns a token that later appears in an error, the unmasked bearer credential can be exposed in the public Actions log. Call maskValue(token, log) immediately after loginUniversalAuth(), and update the multi-line test to locate the token mask rather than relying on fixed masks[0] or masks[1] indices.
Prompt for LLM
File scripts/infisical-fetch.mjs, lines 156-157:
Access token no longer masked, so an error path can leak it to the public log
The Infisical universal-auth access token is no longer registered with the runner's secret masker, leaving it as the only credential in this script that is not covered by an `::add-mask::` command. Any error path that surfaces the token (the `catch` in `resolveVaultValues` logs `err.message`; a future stack/HTTP-error path) writes a live workspace bearer credential into a public Actions log unmasked. Restore the token mask; the new multi-line test must then filter the token mask out instead of asserting on absolute mask indices (`masks[0]`/`masks[1]`), since the token mask precedes the value's lines.
Suggested code:
const token = await loginUniversalAuth({ siteUrl, clientId, clientSecret, fetchImpl });
// The access token rides in an Authorization header below and is never
// printed; mask it anyway so no future error path can surface it.
maskValue(token, log);
return await listSecretsRaw({ siteUrl, token, projectId, environment, secretPath, fetchImpl });
Talk to Kody by mentioning @kody
Was this suggestion helpful? React with 👍 or 👎 to help Kody learn from this interaction.
| function isEntryModule() { | ||
| try { | ||
| return realpathSync(process.argv[1]) === realpathSync(fileURLToPath(import.meta.url)); | ||
| } catch { | ||
| return false; | ||
| } | ||
| } |
There was a problem hiding this comment.
Entry guard fails open on realpath error, skipping run() and exiting 0 silently
When isEntryModule() cannot resolve the entry path, the script skips run() and exits successfully without fetching credentials, so worker-deploy.yml and mac-release.yml can proceed with missing values. On realpathSync failure, fall back to comparing pathToFileURL(process.argv[1]).href with import.meta.url, following the guards in scripts/install-agent-hook-otlp.mjs and scripts/agent-hook-otlp.mjs.
Prompt for LLM
File scripts/infisical-fetch.mjs, lines 220-226:
Entry guard fails open on realpath error, skipping run() and exiting 0 silently
`isEntryModule()` swallows every `realpathSync` failure and returns `false`, so a realpath error makes the script believe it was merely imported. `run()` then never executes, and the process exits 0 having fetched nothing, logged no `::warning::`/`::error::`, and written nothing to `$GITHUB_ENV` — so `worker-deploy.yml` (which sets `INFISICAL_REQUIRED: CLOUDFLARE_FLEET_API_TOKEN`) proceeds with an empty deploy credential instead of failing the required-name check, and `mac-release.yml` proceeds with an unset signing certificate. Follow the repo's own established guard (`scripts/install-agent-hook-otlp.mjs:85-92`, `scripts/agent-hook-otlp.mjs:759-768`) and fall back to the literal comparison instead of reporting "not the entry module".
Suggested code:
import { appendFileSync, realpathSync } from "node:fs";
import { fileURLToPath, pathToFileURL } from "node:url";
function isEntryModule() {
if (!process.argv[1]) return false;
try {
return realpathSync(process.argv[1]) === realpathSync(fileURLToPath(import.meta.url));
} catch {
// argv[1] does not resolve (deleted mid-run, unreadable parent dir) --
// fall back to a literal comparison rather than silently skipping run(),
// which would exit 0 having fetched nothing.
return pathToFileURL(process.argv[1]).href === import.meta.url;
}
}
Talk to Kody by mentioning @kody
Was this suggestion helpful? React with 👍 or 👎 to help Kody learn from this interaction.
| const ROOT = join(dirname(fileURLToPath(import.meta.url)), ".."); | ||
| const SCRIPT = join(ROOT, "scripts", "infisical-fetch.mjs"); |
There was a problem hiding this comment.
New tests never invoke the script through a symlink, so the fix is untested
The existing CLI-entry tests always use the resolved script path, so reverting isEntryModule() to a literal comparison leaves them green while the symlink regression remains uncovered. Add a test that spawns the script through a symlinked directory, verifies the ::warning:: output, and skips Windows because symlink creation may require elevation.
Prompt for LLM
File scripts/infisical-fetch.node-test.mjs, lines 11-12:
New tests never invoke the script through a symlink, so the fix is untested
The script path used by both CLI-entry tests is derived from `import.meta.url`, which Node has already realpath'd, so the child is always spawned through the real path and the pre-change text comparison (`pathToFileURL(argv[1]).href === import.meta.url`) was already true there. The symlinked-invocation regression this PR fixes has no coverage — reverting `isEntryModule()` to the old text comparison leaves every test in this file green, so the headline behavior change is unprotected. Add a test that spawns the script through a symlinked directory and asserts it actually ran (e.g. the `::warning::` on stdout), skipping on Windows where creating symlinks needs elevation.
Suggested code:
const ROOT = join(dirname(fileURLToPath(import.meta.url)), "..");
const SCRIPT = join(ROOT, "scripts", "infisical-fetch.mjs");
test("the CLI entry still runs when the script is invoked through a symlinked directory", (t) => {
if (process.platform === "win32") return t.skip("symlink creation needs elevation on Windows");
const { dir, path } = tempGithubEnv();
try {
const link = join(dir, "linked-repo");
symlinkSync(ROOT, link, "dir");
const result = spawnSync(process.execPath, [join(link, "scripts", "infisical-fetch.mjs")], {
encoding: "utf8",
env: { PATH: process.env.PATH, INFISICAL_NAMES: "BAZ", GITHUB_ENV: path },
});
assert.equal(result.status, 0);
assert.match(result.stdout, /::warning::BAZ is empty/);
} finally {
rmSync(dir, { recursive: true, force: true });
}
});
Talk to Kody by mentioning @kody
Was this suggestion helpful? React with 👍 or 👎 to help Kody learn from this interaction.
Refs AFC-465
Summary
/varto/private/varpath differences.GITHUB_ENVexports, and CLI exit statuses.Test Coverage
Added
scripts/infisical-fetch.node-test.mjs, runnable with:The tests verify that:
GITHUB_ENV.Outstanding Review Items
run()from being invoked while allowing a successful exit. Entry detection should fail closed.