Skip to content

Potential fix for code scanning alert no. 10: Uncontrolled data used in path expression#59

Closed
piquark6046 wants to merge 2 commits intomainfrom
alert-autofix-10
Closed

Potential fix for code scanning alert no. 10: Uncontrolled data used in path expression#59
piquark6046 wants to merge 2 commits intomainfrom
alert-autofix-10

Conversation

@piquark6046
Copy link
Member

Potential fix for https://github.com/FilteringDev/tinyShield/security/code-scanning/10

In general, to fix this kind of issue you should avoid directly trusting environment variables or other unvalidated inputs when constructing filesystem paths. Normalize the path, optionally restrict it to be within an expected root, or fall back to a known-safe value when the input is missing or invalid.

For this specific code, the simplest low-impact fix is:

  • Derive a base directory for the cache from Process.env.INIT_CWD only if it looks like a valid absolute path without suspicious segments; otherwise, fall back to Process.cwd().
  • Use path.resolve to normalize the cache directory and then append '.buildcache' as a path segment instead of string concatenation.
  • This keeps functional behavior the same when INIT_CWD is a normal absolute path, but removes the direct trust in an arbitrary environment string and makes the path construction explicit and safe.

Concretely, in builder/source/cache.ts:

  • Add an import for Node’s path module.
  • Replace the direct string concatenation in CachePath with a small helper that:
    • Chooses a base directory: Process.env.INIT_CWD if it is an absolute path, otherwise Process.cwd().
    • Uses Path.resolve and Path.join to construct CachePath.
  • Leave the rest of the logic (create directory, read/write cache files) unchanged.

No additional methods are strictly required beyond a bit of inlined logic at the constant declaration site, and the only new import needed is the built-in node:path module.

Suggested fixes powered by Copilot Autofix. Review carefully before merging.

…in path expression

Co-authored-by: Copilot Autofix powered by AI <62310815+github-advanced-security[bot]@users.noreply.github.com>
const BaseCacheDir = (Process.env.INIT_CWD && Path.isAbsolute(Process.env.INIT_CWD))
? Process.env.INIT_CWD
: Process.cwd()
const CachePath = Path.join(Path.resolve(BaseCacheDir), '.buildcache')

Check failure

Code scanning / CodeQL

Uncontrolled data used in path expression High

This path depends on a
user-provided value
.

Copilot Autofix

AI about 11 hours ago

General approach: Normalize and validate any environment-derived path before using it to construct filesystem paths, and ensure it cannot escape a designated safe root (ProjectRoot). This matches the pattern described in the background: resolve/realpath the candidate path, then check that it is within the intended root folder. If validation fails, fall back to a safe default (ProjectRoot).

Best concrete fix here: update the computation of EnvInitCwd and BaseCacheDir so that:

  1. We only trust Process.env.INIT_CWD if it is:

    • an absolute path,
    • successfully canonicalized with Fs.realpathSync,
    • and strictly contained within the canonical ProjectRoot directory.
  2. We avoid path traversal or symlink tricks by realpath’ing both ProjectRoot and INIT_CWD and then checking directory containment using Path.relative, which is more robust than startsWith.

  3. If any of these conditions fail, we set EnvInitCwd to null and BaseCacheDir falls back to ProjectRoot as before.

Concretely, in builder/source/cache.ts:

  • Introduce a RealProjectRoot computed via Fs.realpathSync(ProjectRoot), with a safe fallback to ProjectRoot if resolution fails.
  • Replace the current inline ternary that sets EnvInitCwd and the subsequent BaseCacheDir check with logic that:
    • Resolves Process.env.INIT_CWD via Fs.realpathSync only if it’s absolute.
    • Uses Path.relative(RealProjectRoot, realInitCwd) and checks that it is not absolute, does not start with '..', and is not equal to '..'.
    • Only if this containment check passes, sets EnvInitCwd to the realpath’d directory; otherwise, leaves it null.
  • Keep everything else (CachePath, CacheDomainsPath, and the cache read/write functions) unchanged.

No new external dependencies are needed; we already import Fs, Path, and Process.

Suggested changeset 1
builder/source/cache.ts

Autofix patch

Autofix patch
Run the following command in your local git repository to apply this patch
cat << 'EOF' | git apply
diff --git a/builder/source/cache.ts b/builder/source/cache.ts
--- a/builder/source/cache.ts
+++ b/builder/source/cache.ts
@@ -5,12 +5,33 @@
 import { FetchAdShieldDomains } from './references/index.js'
 
 const ProjectRoot = Path.resolve(Process.cwd())
-const EnvInitCwd = Process.env.INIT_CWD && Path.isAbsolute(Process.env.INIT_CWD)
-  ? Path.resolve(Process.env.INIT_CWD)
-  : null
-const BaseCacheDir = (EnvInitCwd && (EnvInitCwd === ProjectRoot || EnvInitCwd.startsWith(ProjectRoot + Path.sep)))
-  ? EnvInitCwd
-  : ProjectRoot
+const RealProjectRoot = (() => {
+  try {
+    return Fs.realpathSync(ProjectRoot)
+  } catch {
+    return ProjectRoot
+  }
+})()
+
+const EnvInitCwd = (() => {
+  const initCwd = Process.env.INIT_CWD
+  if (!initCwd || !Path.isAbsolute(initCwd)) {
+    return null
+  }
+  let realInitCwd: string
+  try {
+    realInitCwd = Fs.realpathSync(initCwd)
+  } catch {
+    return null
+  }
+  const relative = Path.relative(RealProjectRoot, realInitCwd)
+  if (!relative || relative === '.' || (relative && !relative.startsWith('..') && !Path.isAbsolute(relative))) {
+    return realInitCwd
+  }
+  return null
+})()
+
+const BaseCacheDir = EnvInitCwd ?? RealProjectRoot
 const CachePath = Path.join(BaseCacheDir, '.buildcache')
 const CacheDomainsPath = Path.join(CachePath, 'domains.json')
 
EOF
@@ -5,12 +5,33 @@
import { FetchAdShieldDomains } from './references/index.js'

const ProjectRoot = Path.resolve(Process.cwd())
const EnvInitCwd = Process.env.INIT_CWD && Path.isAbsolute(Process.env.INIT_CWD)
? Path.resolve(Process.env.INIT_CWD)
: null
const BaseCacheDir = (EnvInitCwd && (EnvInitCwd === ProjectRoot || EnvInitCwd.startsWith(ProjectRoot + Path.sep)))
? EnvInitCwd
: ProjectRoot
const RealProjectRoot = (() => {
try {
return Fs.realpathSync(ProjectRoot)
} catch {
return ProjectRoot
}
})()

const EnvInitCwd = (() => {
const initCwd = Process.env.INIT_CWD
if (!initCwd || !Path.isAbsolute(initCwd)) {
return null
}
let realInitCwd: string
try {
realInitCwd = Fs.realpathSync(initCwd)
} catch {
return null
}
const relative = Path.relative(RealProjectRoot, realInitCwd)
if (!relative || relative === '.' || (relative && !relative.startsWith('..') && !Path.isAbsolute(relative))) {
return realInitCwd
}
return null
})()

const BaseCacheDir = EnvInitCwd ?? RealProjectRoot
const CachePath = Path.join(BaseCacheDir, '.buildcache')
const CacheDomainsPath = Path.join(CachePath, 'domains.json')

Copilot is powered by AI and may make mistakes. Always verify output.
? Process.env.INIT_CWD
: Process.cwd()
const CachePath = Path.join(Path.resolve(BaseCacheDir), '.buildcache')
const CacheDomainsPath = Path.join(CachePath, 'domains.json')

Check failure

Code scanning / CodeQL

Uncontrolled data used in path expression High

This path depends on a
user-provided value
.

Copilot Autofix

AI about 11 hours ago

In general, to fix uncontrolled path usage derived from environment variables, we should (1) choose a trusted base directory (e.g., the current working directory or a configured application root), (2) resolve any untrusted value against that base using path.resolve, and (3) ensure the resulting path is within the intended directory tree (or fall back to the base if validation fails). For a simple cache directory we can avoid depending on untrusted input entirely, or only use it if it stays within our project root.

For this specific code, the simplest and safest fix without changing observable functionality is:

  • Keep using Process.cwd() as the ultimate trusted base.
  • When INIT_CWD is absolute, resolve it and verify it is inside (or equal to) Process.cwd(). If so, we can use it; otherwise ignore it and fall back to Process.cwd().
  • Alternatively, if we want to eliminate the taint entirely, we can remove use of INIT_CWD and always base the cache on Process.cwd().

To minimize behavioral change while still satisfying CodeQL and hardening security, we’ll validate INIT_CWD against Process.cwd():

  1. Compute const ProjectRoot = Path.resolve(Process.cwd()).
  2. If Process.env.INIT_CWD is set and absolute, compute const initCwd = Path.resolve(Process.env.INIT_CWD).
  3. Check that initCwd starts with ProjectRoot plus a path separator (or is exactly equal). If not, ignore it.
  4. Set BaseCacheDir to the validated initCwd when valid, or to ProjectRoot otherwise.
  5. Build CachePath from BaseCacheDir as before.

All changes are in builder/source/cache.ts at the top where BaseCacheDir and CachePath are defined; no new imports are required because Path is already imported.

Suggested changeset 1
builder/source/cache.ts

Autofix patch

Autofix patch
Run the following command in your local git repository to apply this patch
cat << 'EOF' | git apply
diff --git a/builder/source/cache.ts b/builder/source/cache.ts
--- a/builder/source/cache.ts
+++ b/builder/source/cache.ts
@@ -4,10 +4,14 @@
 import * as Path from 'node:path'
 import { FetchAdShieldDomains } from './references/index.js'
 
-const BaseCacheDir = (Process.env.INIT_CWD && Path.isAbsolute(Process.env.INIT_CWD))
-  ? Process.env.INIT_CWD
-  : Process.cwd()
-const CachePath = Path.join(Path.resolve(BaseCacheDir), '.buildcache')
+const ProjectRoot = Path.resolve(Process.cwd())
+const EnvInitCwd = Process.env.INIT_CWD && Path.isAbsolute(Process.env.INIT_CWD)
+  ? Path.resolve(Process.env.INIT_CWD)
+  : null
+const BaseCacheDir = (EnvInitCwd && (EnvInitCwd === ProjectRoot || EnvInitCwd.startsWith(ProjectRoot + Path.sep)))
+  ? EnvInitCwd
+  : ProjectRoot
+const CachePath = Path.join(BaseCacheDir, '.buildcache')
 const CacheDomainsPath = Path.join(CachePath, 'domains.json')
 
 export function CreateCache(Domains: Set<string>) {
EOF
@@ -4,10 +4,14 @@
import * as Path from 'node:path'
import { FetchAdShieldDomains } from './references/index.js'

const BaseCacheDir = (Process.env.INIT_CWD && Path.isAbsolute(Process.env.INIT_CWD))
? Process.env.INIT_CWD
: Process.cwd()
const CachePath = Path.join(Path.resolve(BaseCacheDir), '.buildcache')
const ProjectRoot = Path.resolve(Process.cwd())
const EnvInitCwd = Process.env.INIT_CWD && Path.isAbsolute(Process.env.INIT_CWD)
? Path.resolve(Process.env.INIT_CWD)
: null
const BaseCacheDir = (EnvInitCwd && (EnvInitCwd === ProjectRoot || EnvInitCwd.startsWith(ProjectRoot + Path.sep)))
? EnvInitCwd
: ProjectRoot
const CachePath = Path.join(BaseCacheDir, '.buildcache')
const CacheDomainsPath = Path.join(CachePath, 'domains.json')

export function CreateCache(Domains: Set<string>) {
Copilot is powered by AI and may make mistakes. Always verify output.
@piquark6046 piquark6046 committed this autofix suggestion about 11 hours ago.
…in path expression

Co-authored-by: Copilot Autofix powered by AI <62310815+github-advanced-security[bot]@users.noreply.github.com>
? Path.resolve(Process.env.INIT_CWD)
: null
const BaseCacheDir = (EnvInitCwd && (EnvInitCwd === ProjectRoot || EnvInitCwd.startsWith(ProjectRoot + Path.sep)))
? EnvInitCwd

Check failure

Code scanning / CodeQL

Uncontrolled data used in path expression High

This path depends on a
user-provided value
.

Copilot Autofix

AI about 11 hours ago

To fix this, we should ensure that any environment-derived path is safely normalized and verified to be within the intended root directory before it is used. That means resolving EnvInitCwd and ProjectRoot to absolute, normalized paths and then ensuring that EnvInitCwd is actually contained within ProjectRoot using a robust check, rather than a simple string startsWith on potentially unnormalized values.

The best minimal fix here is:

  • Normalize ProjectRoot and any candidate EnvInitCwd using Path.resolve.
  • Introduce a small helper function (in this file) isSubPath(parent, child) that:
    • Resolves both arguments.
    • Compares them in a path-separator-aware way, e.g. child === parent or child.startsWith(parent + Path.sep).
  • Use this helper to decide whether to trust EnvInitCwd; otherwise fall back to ProjectRoot.

This keeps behavior the same for normal, valid values of INIT_CWD, but eliminates cases where a crafted path that normalizes outside ProjectRoot or relies on path quirks could be accepted. All required imports (Path, Process) already exist, so we only need to add the helper and adjust the BaseCacheDir computation within builder/source/cache.ts.

Suggested changeset 1
builder/source/cache.ts

Autofix patch

Autofix patch
Run the following command in your local git repository to apply this patch
cat << 'EOF' | git apply
diff --git a/builder/source/cache.ts b/builder/source/cache.ts
--- a/builder/source/cache.ts
+++ b/builder/source/cache.ts
@@ -5,10 +5,17 @@
 import { FetchAdShieldDomains } from './references/index.js'
 
 const ProjectRoot = Path.resolve(Process.cwd())
+
+function isSubPath(parent: string, child: string): boolean {
+  const parentResolved = Path.resolve(parent)
+  const childResolved = Path.resolve(child)
+  return childResolved === parentResolved || childResolved.startsWith(parentResolved + Path.sep)
+}
+
 const EnvInitCwd = Process.env.INIT_CWD && Path.isAbsolute(Process.env.INIT_CWD)
   ? Path.resolve(Process.env.INIT_CWD)
   : null
-const BaseCacheDir = (EnvInitCwd && (EnvInitCwd === ProjectRoot || EnvInitCwd.startsWith(ProjectRoot + Path.sep)))
+const BaseCacheDir = (EnvInitCwd && isSubPath(ProjectRoot, EnvInitCwd))
   ? EnvInitCwd
   : ProjectRoot
 const CachePath = Path.join(BaseCacheDir, '.buildcache')
EOF
@@ -5,10 +5,17 @@
import { FetchAdShieldDomains } from './references/index.js'

const ProjectRoot = Path.resolve(Process.cwd())

function isSubPath(parent: string, child: string): boolean {
const parentResolved = Path.resolve(parent)
const childResolved = Path.resolve(child)
return childResolved === parentResolved || childResolved.startsWith(parentResolved + Path.sep)
}

const EnvInitCwd = Process.env.INIT_CWD && Path.isAbsolute(Process.env.INIT_CWD)
? Path.resolve(Process.env.INIT_CWD)
: null
const BaseCacheDir = (EnvInitCwd && (EnvInitCwd === ProjectRoot || EnvInitCwd.startsWith(ProjectRoot + Path.sep)))
const BaseCacheDir = (EnvInitCwd && isSubPath(ProjectRoot, EnvInitCwd))
? EnvInitCwd
: ProjectRoot
const CachePath = Path.join(BaseCacheDir, '.buildcache')
Copilot is powered by AI and may make mistakes. Always verify output.
? EnvInitCwd
: ProjectRoot
const CachePath = Path.join(BaseCacheDir, '.buildcache')
const CacheDomainsPath = Path.join(CachePath, 'domains.json')

Check failure

Code scanning / CodeQL

Uncontrolled data used in path expression High

This path depends on a
user-provided value
.

Copilot Autofix

AI about 11 hours ago

In general, to fix uncontrolled path use when combining a root directory with user-controlled components, always normalize the candidate path (using path.resolve and ideally fs.realpathSync), and then verify that the normalized path is within an expected root (via a robust prefix check) before using it. Never rely on naive string concatenation or startsWith checks on unnormalized paths.

For this specific code, the intent is to allow INIT_CWD to point somewhere within the project directory tree and, if it does, to use that as the base for .buildcache; otherwise, default to ProjectRoot. The best way to fix this without changing behavior is:

  1. Normalize Process.env.INIT_CWD with Path.resolve before any containment check.
  2. Optionally call Fs.realpathSync (wrapped in a try/catch) to resolve symlinks; if resolution fails, treat it as invalid and ignore INIT_CWD.
  3. Perform the containment check on the normalized path: ensure it is equal to ProjectRoot or is strictly inside it. For the “inside” case, a robust check is: normalizedPath === ProjectRoot || (normalizedPath.startsWith(ProjectRoot + Path.sep)).
  4. Only if the check passes, use that normalized value as BaseCacheDir; otherwise, set BaseCacheDir to ProjectRoot.

We can implement this by slightly restructuring the EnvInitCwd / BaseCacheDir initialization block at the top of builder/source/cache.ts. No new external dependencies are needed; we can reuse node:fs and node:path, which are already imported. The rest of the logic using CachePath and CacheDomainsPath can remain unchanged.

Suggested changeset 1
builder/source/cache.ts

Autofix patch

Autofix patch
Run the following command in your local git repository to apply this patch
cat << 'EOF' | git apply
diff --git a/builder/source/cache.ts b/builder/source/cache.ts
--- a/builder/source/cache.ts
+++ b/builder/source/cache.ts
@@ -5,8 +5,16 @@
 import { FetchAdShieldDomains } from './references/index.js'
 
 const ProjectRoot = Path.resolve(Process.cwd())
-const EnvInitCwd = Process.env.INIT_CWD && Path.isAbsolute(Process.env.INIT_CWD)
-  ? Path.resolve(Process.env.INIT_CWD)
+const EnvInitCwdRaw = Process.env.INIT_CWD
+const EnvInitCwd = EnvInitCwdRaw && Path.isAbsolute(EnvInitCwdRaw)
+  ? (() => {
+    const resolved = Path.resolve(EnvInitCwdRaw)
+    try {
+      return Fs.realpathSync(resolved)
+    } catch {
+      return null
+    }
+  })()
   : null
 const BaseCacheDir = (EnvInitCwd && (EnvInitCwd === ProjectRoot || EnvInitCwd.startsWith(ProjectRoot + Path.sep)))
   ? EnvInitCwd
EOF
@@ -5,8 +5,16 @@
import { FetchAdShieldDomains } from './references/index.js'

const ProjectRoot = Path.resolve(Process.cwd())
const EnvInitCwd = Process.env.INIT_CWD && Path.isAbsolute(Process.env.INIT_CWD)
? Path.resolve(Process.env.INIT_CWD)
const EnvInitCwdRaw = Process.env.INIT_CWD
const EnvInitCwd = EnvInitCwdRaw && Path.isAbsolute(EnvInitCwdRaw)
? (() => {
const resolved = Path.resolve(EnvInitCwdRaw)
try {
return Fs.realpathSync(resolved)
} catch {
return null
}
})()
: null
const BaseCacheDir = (EnvInitCwd && (EnvInitCwd === ProjectRoot || EnvInitCwd.startsWith(ProjectRoot + Path.sep)))
? EnvInitCwd
Copilot is powered by AI and may make mistakes. Always verify output.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant