trending/teamai-cli
1
0
Fork 0
mirror of https://github.com/Tencent/teamai-cli.git synced 2026-10-02 03:14:40 +08:00
Code Issues Packages Projects Releases Wiki Activity

fix(env): only trust verifiable && and || conditions, ignore if bodies (review)

Browse Source 
Two more P1s from the bot's round-11 review of aaa1142, both about
referencesCandidate() trusting shell control flow it can't actually
evaluate:

- Any `&&` was treated as making its right side reachable, without
  checking what the left side's condition even was. A guard like
  `[ "$TERM_PROGRAM" = vscode ] && source ~/.bashrc` would mark
  .bashrc reachable unconditionally, even though it only runs inside
  VS Code. Also flagged: a source sitting inside a multiline `if`
  body looks, line by line, identical to a top-level one.

- Folding `||`'s right side into its left statement (the round-9 fix)
  went too conservative the other way: `source ~/.profile ||
  source ~/.bashrc` DOES guarantee .bashrc runs when ~/.profile
  doesn't exist, and the resolver was never even trying it.

Rather than growing another ad hoc regex tweak, rewrote
referencesCandidate() around what it can actually verify without a
real shell parser:

  - Unconditional: a bare `. REF` / `source REF` — but nothing inside
    an `if` block counts, conditional or not. An `if`'s condition is
    opaque to a line scanner; trusting some conditions and not others
    would just be guessing.
  - Existence-gated `&&`: only the self-referential idiom
    `test -f REF && . REF` / `[ -f REF ] && . REF`, where the tested
    path and the sourced path are the same candidate — the one `&&`
    condition this code can independently verify, by visiting that
    candidate itself later in the search.
  - `||` fallback: the left side always counts (always attempted);
    the right side counts only when the left side's own target file
    does not exist on disk — the one case an `||` fallback is
    actually guaranteed to run.

Anything this can't resolve either way is never trusted: the search
just doesn't queue that candidate, and the caller falls back to the
order-based pick — at worst a harmless duplicate block (the
pre-#693-fix behavior), never a false "reachable" that would
reintroduce #682.

Verified end-to-end on a real Windows host: re-ran the core
Git-for-Windows two-pull scenario from #693 (self-referential &&,
still recognized) with no regression. Added 3 unit tests for the new
boundaries: || recognized when the left target is missing, a
non-existence && condition rejected, and a source nested inside an
if block rejected.

Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
This commit is contained in:
STiFLeR7
2026-09-23 09:46:33 +05:30
co-authored by Claude Sonnet 5
parent aaa1142729
commit 60a2da0252
2 changed files with 117 additions and 30 deletions
Download Patch File Download Diff File Expand all files Collapse all files
src/__tests__/shell-profile.test.ts
+43
View File
@@ -248,6 +248,49 @@ describe('resolveActiveShellProfile', () => {
await fse.writeFile(path.join(homeDir, '.bashrc'), teamaiBlock());
expect(await resolveActiveShellProfile(envShPath, 'win32')).toBe(path.join(homeDir, '.profile'));
});
// Regression (#693 review round 11): the right side of `||` genuinely is
// guaranteed to run when the left side's own target file does not exist —
// the one case this scanner can verify without a real shell. Failing to
// recognize it falls back to injecting a duplicate, which round 10's fix
// was meant to avoid for exactly this shape of line.
it('does treat the right side of || as reachable when the left side\'s target is missing', async () => {
await fse.writeFile(
path.join(homeDir, '.bash_profile'),
'source ~/.profile || source ~/.bashrc\n',
);
// .profile is deliberately absent.
await fse.writeFile(path.join(homeDir, '.bashrc'), teamaiBlock());
expect(await resolveActiveShellProfile(envShPath, 'win32')).toBe(path.join(homeDir, '.bashrc'));
});
// Regression (#693 review round 11): `&&` only establishes reachability
// when this scanner can independently verify the guarding condition — the
// self-referential existence test. A condition testing anything else
// (here, an environment variable) is not verifiable, so a stale block
// behind it must not outrank a genuinely unwritten, currently-read file.
it('does not treat a non-existence && condition as reachable', async () => {
await fse.writeFile(
path.join(homeDir, '.bash_profile'),
'[ "$TERM_PROGRAM" = vscode ] && source ~/.bashrc\n',
);
await fse.writeFile(path.join(homeDir, '.bashrc'), teamaiBlock());
expect(await resolveActiveShellProfile(envShPath, 'win32')).toBe(path.join(homeDir, '.bash_profile'));
});
// Regression (#693 review round 11): a source line's own text looks
// identical whether it sits at top level or three lines inside an `if`
// block this scanner cannot evaluate. Nothing inside an `if` is trusted,
// conditional or not, so a block only reachable through one is not
// preferred over the order-based pick.
it('does not treat a source nested inside an if block as reachable', async () => {
await fse.writeFile(
path.join(homeDir, '.bash_profile'),
'if [ -n "$BASH_VERSION" ]; then\n . ~/.bashrc\nfi\n',
);
await fse.writeFile(path.join(homeDir, '.bashrc'), teamaiBlock());
expect(await resolveActiveShellProfile(envShPath, 'win32')).toBe(path.join(homeDir, '.bash_profile'));
});
});
describe('envBlockSourcesPath', () => {
src/utils/shell-profile.ts
+74 -30
View File
@@ -202,38 +202,82 @@ export function envBlockReferencesDataHome(block: string, envShPath: string): bo
return false;
}
/**
* Whether `content` runs a `source`/`.` command on a home-relative reference
* to `name` (`~/.bashrc`, `$HOME/.bashrc`, `${HOME}/.bashrc`) — the shape a
* real forwarding line takes, e.g. Git for Windows' generated
* `test -f ~/.bashrc && . ~/.bashrc`.
*
* Deliberately narrower than a substring search (#693 review round 9): that
* matched a comment mentioning the filename (inert, never executed) and a
* same-prefixed but different file (`~/.bashrc.local` contains `~/.bashrc`
* as a substring). Comment lines are dropped outright; each remaining line
* is split on `&&`/`||`/`;` into statements, and a statement only counts
* when its first word is literally `.` or `source` and its second word is
* exactly the home-relative reference — anchored, so a longer filename
* cannot satisfy it by prefix.
*/
function referencesCandidate(content: string, name: string): boolean {
/** `~/name`, `$HOME/name` or `${HOME}/name`, optionally quoted, as a token this scanner accepts as a reference to `name`. */
function homeRelativeRef(name: string): string {
const escaped = name.replace(/[.*+?^${}()|[\]\\]/g, '\\$&');
const target = new RegExp(`^["']?(?:~|\\$\\{?HOME\\}?)/${escaped}(?![\\w.-])["']?$`);
return `["']?(?:~|\\$\\{?HOME\\}?)/${escaped}(?![\\w.-])["']?`;
}
/** If `token` is a home-relative reference (`~/name`, `$HOME/name`, `${HOME}/name`), the real path it names. */
function homeRelativePath(token: string, home: string): string | null {
const stripped = token.replace(/^["']/, '').replace(/["']$/, '');
const m = stripped.match(/^(?:~|\$\{?HOME\}?)\/(.+)$/);
return m ? path.join(home, m[1]) : null;
}
/**
* Whether `content` runs a `source`/`.` command reaching `name`
* (`~/.bashrc`, `$HOME/.bashrc`, `${HOME}/.bashrc`), restricted to the forms
* this scanner can reason about without a real shell parser (#693 review
* round 11 named two more gaps a bare substring/`&&`/`;` split left open):
*
* - **Unconditional**: a bare `. REF` / `source REF`, as its own `;`-joined
* statement. Nothing inside an `if` block counts, conditional or not —
* the condition is opaque to a line scanner, so a source sitting three
* lines under `if [ -n "$BASH_VERSION" ]; then` is no more verifiable
* than one under `if [ "$TERM_PROGRAM" = vscode ]; then`, and trusting
* either would risk the same false "reachable" #682 regression the
* sticky resolver exists to prevent.
* - **Existence-gated**: `test -f REF && . REF` / `[ -f REF ] && . REF`,
* self-referential only — the tested path and the sourced path must both
* be `name`, the one condition this scanner can independently verify (by
* visiting that candidate itself later in the search). A condition
* testing anything else grants nothing.
* - **`||` fallback**: `A || B`, where `A` is a source of some other
* candidate. The left side of `||` is always attempted, so it counts
* unconditionally; the right side runs only if the left one fails, which
* is verifiable in exactly one case — `A`'s own target does not exist on
* disk — so `B` counts only then.
*
* A file, or a shell construct, this cannot resolve one way or the other is
* never trusted either way: the caller falls back to the order-based pick,
* which is safe (at worst a harmless duplicate block, the pre-#693-fix
* behavior) — never a false "reachable" that would silently reintroduce
* #682.
*/
async function referencesCandidate(content: string, name: string, home: string): Promise<boolean> {
const ref = homeRelativeRef(name);
const refOnly = new RegExp(`^${ref}$`);
const existenceGated = new RegExp(
`^(?:test\\s+-f\\s+${ref}|\\[\\s+-f\\s+${ref}\\s*\\])\\s*&&\\s*(?:\\.|source)\\s+${ref}$`,
);
const sourceOf = /^(?:\.|source)\s+(\S+)$/;
let ifDepth = 0;
for (const rawLine of content.split('\n')) {
if (rawLine.trimStart().startsWith('#')) continue;
// Only `&&`/`;` split into statements that are still unconditionally
// attempted (or gated on the referenced candidate's own existence, which
// is independently re-checked by reading that candidate). `||`'s right
// side runs only if its left side fails — something not established
// here — so it is left folded into the same statement as its left side:
// that statement's first `.`/`source` command (the unconditional one)
// still matches, but a `source` sitting only after `||` never does.
for (const statement of rawLine.split(/&&|;/)) {
const tokens = statement.trim().split(/\s+/);
if (tokens.length >= 2 && (tokens[0] === '.' || tokens[0] === 'source') && target.test(tokens[1])) {
return true;
const line = rawLine.trim();
if (!line || line.startsWith('#')) continue;
if (/^if\b/.test(line)) { ifDepth += 1; continue; }
if (/^fi\b/.test(line)) { ifDepth = Math.max(0, ifDepth - 1); continue; }
if (ifDepth > 0) continue;
for (const statement of line.split(';').map((s) => s.trim()).filter(Boolean)) {
if (existenceGated.test(statement)) return true;
const orParts = statement.split('||').map((s) => s.trim());
if (orParts.length === 2) {
const left = orParts[0].match(sourceOf);
const right = orParts[1].match(sourceOf);
if (left && refOnly.test(left[1])) return true;
if (left && right && refOnly.test(right[1])) {
const leftPath = homeRelativePath(left[1], home);
if (leftPath && !(await pathExists(leftPath))) return true;
}
continue;
}
const plain = statement.match(sourceOf);
if (plain && refOnly.test(plain[1])) return true;
}
}
return false;
@@ -291,7 +335,7 @@ export async function resolveActiveShellProfile(
for (const name of SHELL_PROFILE_CANDIDATE_NAMES) {
const candidate = path.join(home, name);
if (candidate !== current && !visited.has(candidate) && referencesCandidate(content, name)) {
if (candidate !== current && !visited.has(candidate) && await referencesCandidate(content, name, home)) {
queue.push(candidate);
}
}