Potential fix for code scanning alert no. 10: Uncontrolled data used in path expression#59
Potential fix for code scanning alert no. 10: Uncontrolled data used in path expression#59piquark6046 wants to merge 2 commits intomainfrom
Conversation
…in path expression Co-authored-by: Copilot Autofix powered by AI <62310815+github-advanced-security[bot]@users.noreply.github.com>
builder/source/cache.ts
Outdated
| 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
Show autofix suggestion
Hide autofix suggestion
Copilot Autofix
AI about 16 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:
-
We only trust
Process.env.INIT_CWDif it is:- an absolute path,
- successfully canonicalized with
Fs.realpathSync, - and strictly contained within the canonical
ProjectRootdirectory.
-
We avoid path traversal or symlink tricks by realpath’ing both
ProjectRootandINIT_CWDand then checking directory containment usingPath.relative, which is more robust thanstartsWith. -
If any of these conditions fail, we set
EnvInitCwdtonullandBaseCacheDirfalls back toProjectRootas before.
Concretely, in builder/source/cache.ts:
- Introduce a
RealProjectRootcomputed viaFs.realpathSync(ProjectRoot), with a safe fallback toProjectRootif resolution fails. - Replace the current inline ternary that sets
EnvInitCwdand the subsequentBaseCacheDircheck with logic that:- Resolves
Process.env.INIT_CWDviaFs.realpathSynconly 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
EnvInitCwdto the realpath’d directory; otherwise, leaves itnull.
- Resolves
- 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.
| @@ -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') | ||
|
|
| ? 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 autofix suggestion was applied.
Show autofix suggestion
Hide autofix suggestion
Copilot Autofix
AI about 16 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_CWDis 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 toProcess.cwd(). - Alternatively, if we want to eliminate the taint entirely, we can remove use of
INIT_CWDand always base the cache onProcess.cwd().
To minimize behavioral change while still satisfying CodeQL and hardening security, we’ll validate INIT_CWD against Process.cwd():
- Compute
const ProjectRoot = Path.resolve(Process.cwd()). - If
Process.env.INIT_CWDis set and absolute, computeconst initCwd = Path.resolve(Process.env.INIT_CWD). - Check that
initCwdstarts withProjectRootplus a path separator (or is exactly equal). If not, ignore it. - Set
BaseCacheDirto the validatedinitCwdwhen valid, or toProjectRoototherwise. - Build
CachePathfromBaseCacheDiras 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.
| @@ -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>) { |
…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
Show autofix suggestion
Hide autofix suggestion
Copilot Autofix
AI about 16 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
ProjectRootand any candidateEnvInitCwdusingPath.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 === parentorchild.startsWith(parent + Path.sep).
- Use this helper to decide whether to trust
EnvInitCwd; otherwise fall back toProjectRoot.
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.
| @@ -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') |
| ? 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
Show autofix suggestion
Hide autofix suggestion
Copilot Autofix
AI about 16 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:
- Normalize
Process.env.INIT_CWDwithPath.resolvebefore any containment check. - Optionally call
Fs.realpathSync(wrapped in atry/catch) to resolve symlinks; if resolution fails, treat it as invalid and ignoreINIT_CWD. - Perform the containment check on the normalized path: ensure it is equal to
ProjectRootor is strictly inside it. For the “inside” case, a robust check is:normalizedPath === ProjectRoot || (normalizedPath.startsWith(ProjectRoot + Path.sep)). - Only if the check passes, use that normalized value as
BaseCacheDir; otherwise, setBaseCacheDirtoProjectRoot.
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.
| @@ -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 |
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:
Process.env.INIT_CWDonly if it looks like a valid absolute path without suspicious segments; otherwise, fall back toProcess.cwd().path.resolveto normalize the cache directory and then append'.buildcache'as a path segment instead of string concatenation.INIT_CWDis 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:pathmodule.CachePathwith a small helper that:Process.env.INIT_CWDif it is an absolute path, otherwiseProcess.cwd().Path.resolveandPath.jointo constructCachePath.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:pathmodule.Suggested fixes powered by Copilot Autofix. Review carefully before merging.