security: config include path hardening (NUL + length)

[maweibin]

Summary

This PR adds path hardening for the config $include directive (CWE-22 defense-in-depth). It does not introduce new features; it tightens validation so that invalid or abusive include paths are rejected early with clear errors instead of relying on platform-dependent or undefined behavior.

Motivation

• NUL bytes (\0): On some platforms or when passed to native/fs APIs, a path containing a null byte can be interpreted as truncated (e.g. "./a\0../../../etc/passwd" → "./a"), which could bypass path-traversal checks or cause inconsistent behavior. Rejecting NUL in include paths removes this ambiguity.
• Path length: Unbounded include paths (or resolved absolute paths) can cause performance issues, stack pressure, or hit OS limits in undefined ways. Capping at 4096 characters (aligned with common PATH_MAX) keeps behavior predictable and avoids DoS-style inputs.

Existing protections (path traversal and symlink checks) remain unchanged; this PR adds input validation before path resolution.

Changes

Code (src/config/includes.ts)

• New constant: MAX_INCLUDE_PATH_LENGTH = 4096 (exported for tests and possible reuse).
• In resolvePath(includePath) (before any path.resolve / path.normalize):
  1. If includePath contains \0 → throw ConfigIncludeError with message: "Include path must not contain null bytes".
  2. If includePath.length > MAX_INCLUDE_PATH_LENGTH → throw ConfigIncludeError with message: "Include path exceeds maximum length (4096 characters)".
  3. After computing normalized = path.normalize(resolved), if normalized.length > MAX_INCLUDE_PATH_LENGTH → throw ConfigIncludeError with message: "Resolved include path exceeds maximum length (4096 characters)".

All new errors use the existing ConfigIncludeError type and do not log or echo the raw path in the message (to avoid leaking NUL or very long strings).

Tests (src/config/includes.test.ts)

• Import MAX_INCLUDE_PATH_LENGTH.
• Malformed paths: Expect ConfigIncludeError with message matching /null bytes?/i for paths containing \0 (e.g. "./file\x00.json", "./a\x00b.json"); keep existing test for //etc/passwd (path traversal).
• Length limit: New test "rejects include path exceeding maximum length" — paths of length MAX_INCLUDE_PATH_LENGTH + 1 and exactly MAX_INCLUDE_PATH_LENGTH (which can still produce a resolved path over the limit) both throw with a message containing "maximum length".
• Regression: New test "accepts include path at or under maximum length when file exists" — normal short paths still resolve correctly.

Documentation

• docs/gateway/configuration-reference.md (Config includes section): Document that paths must not contain null bytes and must not exceed 4096 characters (before or after resolution); add "invalid path format (null bytes or excessive length)" to the Errors bullet.
• docs/gateway/configuration.md ($include accordion): Add that paths must not contain null bytes and must not exceed 4096 characters; extend error handling to include invalid path format.

Is this a feature?

No. This is security hardening and specification of existing behavior:

• No new user-facing capability is added.
• Previously, NUL or very long paths could lead to implementation-defined or platform-dependent behavior; now they are explicitly rejected with a clear error. Normal, valid include paths are unchanged.

Testing

• pnpm test src/config/includes.test.ts — all 28 tests pass.
• No changes to other config or security tests.

Checklist

• Code: NUL and length checks in resolvePath; clear ConfigIncludeError messages.
• Tests: NUL, length limit, and regression cases.
• Docs: configuration-reference and configuration updated for path validation and error behavior.
• No new optional behavior; only validation and error messages.

[greptile-apps]

Greptile Summary

This PR adds input-validation hardening to the $include path resolution logic in src/config/includes.ts, rejecting paths containing NUL bytes and paths whose length exceeds 4096 characters (before and after resolution). It is a defence-in-depth measure that complements the existing path-traversal and symlink checks, not a user-facing feature.

• NUL-byte rejection is correct and well-motivated (platform-dependent truncation risk).
• Length guards are correctly placed before any path.resolve/path.normalize call.
• Off-by-one boundary: Both guards use strict >, so a path whose length is exactly MAX_INCLUDE_PATH_LENGTH (4096 chars) passes both checks. For a relative path this is harmless because resolution always adds the configDir prefix, but an absolute path of exactly 4096 characters is never lengthened by normalization, meaning it bypasses both guards and reaches the OS, where Linux may return ENAMETOOLONG. Using >= (or reducing MAX_INCLUDE_PATH_LENGTH to 4095) would close this gap.
• The atLimit test covers only the resolved-path guard (via a relative path); the boundary case for absolute 4096-char paths is not tested.
• Documentation updates in configuration-reference.md and configuration.md are accurate and consistent with the code.

Confidence Score: 3/5

• Safe to merge with low risk, but the strict > boundary allows absolute paths of exactly 4096 characters to bypass both length guards.
• The NUL-byte check and relative-path length guard work correctly in practice. The only substantive concern is that absolute paths of exactly MAX_INCLUDE_PATH_LENGTH characters pass both guards unchanged and reach the OS, potentially causing ENAMETOOLONG on Linux (where PATH_MAX = 4096 includes the null terminator). The existing path-traversal check still limits damage, but the length invariant is not fully enforced for absolute paths at the boundary. Changing > to >= would resolve this without any other side effects.
• Pay attention to the boundary condition in src/config/includes.ts at the two normalized.length > MAX_INCLUDE_PATH_LENGTH and includePath.length > MAX_INCLUDE_PATH_LENGTH guards, and the corresponding test in src/config/includes.test.ts.

Prompt To Fix All With AI

This is a comment left during a code review.
Path: src/config/includes.ts
Line: 198-216

Comment:
**Off-by-one allows exactly 4096-char absolute paths to bypass both checks**

Both length guards use strict greater-than (`>`), so a path whose character count is *exactly* `MAX_INCLUDE_PATH_LENGTH` (4096) passes them both. For a **relative** path this is fine because resolution prepends `configDir`, making the resulting path longer. But for an **absolute** path that is already 4096 characters long, normalization never increases the length, so both checks read `4096 > 4096 → false` and the path reaches `fs.realpathSync` / `fs.readFileSync`.

On Linux `PATH_MAX = 4096` *includes* the null terminator, so paths of exactly 4096 characters can trigger `ENAMETOOLONG`. The stated goal is "capping at 4096", which implies paths of that length should be rejected.

Consider using `>=` on both guards (or keeping the constant at 4096 but treating it as an exclusive upper bound in the error message):

```suggestion
    if (includePath.length > MAX_INCLUDE_PATH_LENGTH) {
```

```suggestion
    if (normalized.length > MAX_INCLUDE_PATH_LENGTH) {
```

These two lines should be changed to `>=` to correctly exclude 4096-char paths:

```ts
    if (includePath.length >= MAX_INCLUDE_PATH_LENGTH) {
      throw new ConfigIncludeError(
        `Include path exceeds maximum length (${MAX_INCLUDE_PATH_LENGTH} characters)`,
        includePath,
      );
    }
    // ...
    if (normalized.length >= MAX_INCLUDE_PATH_LENGTH) {
      throw new ConfigIncludeError(
        `Resolved include path exceeds maximum length (${MAX_INCLUDE_PATH_LENGTH} characters)`,
        includePath,
      );
    }
```

How can I resolve this? If you propose a fix, please make it concise.

---

This is a comment left during a code review.
Path: src/config/includes.test.ts
Line: 614-615

Comment:
**"atLimit" case tests resolved-path limit, not raw-path limit — absolute paths at the limit are untested**

`"b".repeat(MAX_INCLUDE_PATH_LENGTH)` (exactly 4096 `b`s) is a *relative* path, so it gets resolved to `configDir + "/" + "b".repeat(4096)`, which is always longer than 4096 characters and is caught by the *second* (post-normalize) check. The *first* check (`includePath.length > MAX_INCLUDE_PATH_LENGTH`) is never exercised for boundary value 4096.

More importantly, an **absolute** path of exactly 4096 characters is never covered. For that case, the resolved path equals the input, normalization leaves it unchanged, and both guards evaluate to `4096 > 4096 → false` — so the path passes through to the OS. Consider adding a test:

```ts
// absolute path at exactly the limit: should also be rejected
const absoluteAtLimit = "/" + "a".repeat(MAX_INCLUDE_PATH_LENGTH - 1); // length == 4096
expectResolveIncludeError(
  () => resolve({ $include: absoluteAtLimit }, {}),
  /maximum length/,
);
```

If the boundary is intentionally exclusive (i.e., 4096-char paths are permitted), the test name and PR description should be updated to reflect that.

How can I resolve this? If you propose a fix, please make it concise.

_{Last reviewed commit: f543dae}

[greptile-apps]

Off-by-one allows exactly 4096-char absolute paths to bypass both checks

Both length guards use strict greater-than (>), so a path whose character count is exactly MAX_INCLUDE_PATH_LENGTH (4096) passes them both. For a relative path this is fine because resolution prepends configDir, making the resulting path longer. But for an absolute path that is already 4096 characters long, normalization never increases the length, so both checks read 4096 > 4096 → false and the path reaches fs.realpathSync / fs.readFileSync.

On Linux PATH_MAX = 4096 includes the null terminator, so paths of exactly 4096 characters can trigger ENAMETOOLONG. The stated goal is "capping at 4096", which implies paths of that length should be rejected.

Consider using >= on both guards (or keeping the constant at 4096 but treating it as an exclusive upper bound in the error message):

Suggested change

	if (includePath.length > MAX_INCLUDE_PATH_LENGTH) {
	throw new ConfigIncludeError(
	`Include path exceeds maximum length (${MAX_INCLUDE_PATH_LENGTH} characters)`,
	includePath,
	);
	}
	const configDir = path.dirname(this.basePath);
	const resolved = path.isAbsolute(includePath)
	? includePath
	: path.resolve(configDir, includePath);
	const normalized = path.normalize(resolved);
	if (normalized.length > MAX_INCLUDE_PATH_LENGTH) {
	throw new ConfigIncludeError(
	`Resolved include path exceeds maximum length (${MAX_INCLUDE_PATH_LENGTH} characters)`,
	includePath,
	);
	}
	if (includePath.length > MAX_INCLUDE_PATH_LENGTH) {

Suggested change

	if (includePath.length > MAX_INCLUDE_PATH_LENGTH) {
	throw new ConfigIncludeError(
	`Include path exceeds maximum length (${MAX_INCLUDE_PATH_LENGTH} characters)`,
	includePath,
	);
	}
	const configDir = path.dirname(this.basePath);
	const resolved = path.isAbsolute(includePath)
	? includePath
	: path.resolve(configDir, includePath);
	const normalized = path.normalize(resolved);
	if (normalized.length > MAX_INCLUDE_PATH_LENGTH) {
	throw new ConfigIncludeError(
	`Resolved include path exceeds maximum length (${MAX_INCLUDE_PATH_LENGTH} characters)`,
	includePath,
	);
	}
	if (normalized.length > MAX_INCLUDE_PATH_LENGTH) {

These two lines should be changed to >= to correctly exclude 4096-char paths:

    if (includePath.length >= MAX_INCLUDE_PATH_LENGTH) {
      throw new ConfigIncludeError(
        `Include path exceeds maximum length (${MAX_INCLUDE_PATH_LENGTH} characters)`,
        includePath,
      );
    }
    // ...
    if (normalized.length >= MAX_INCLUDE_PATH_LENGTH) {
      throw new ConfigIncludeError(
        `Resolved include path exceeds maximum length (${MAX_INCLUDE_PATH_LENGTH} characters)`,
        includePath,
      );
    }

Prompt To Fix With AI

This is a comment left during a code review.
Path: src/config/includes.ts
Line: 198-216

Comment:
**Off-by-one allows exactly 4096-char absolute paths to bypass both checks**

Both length guards use strict greater-than (`>`), so a path whose character count is *exactly* `MAX_INCLUDE_PATH_LENGTH` (4096) passes them both. For a **relative** path this is fine because resolution prepends `configDir`, making the resulting path longer. But for an **absolute** path that is already 4096 characters long, normalization never increases the length, so both checks read `4096 > 4096 → false` and the path reaches `fs.realpathSync` / `fs.readFileSync`.

On Linux `PATH_MAX = 4096` *includes* the null terminator, so paths of exactly 4096 characters can trigger `ENAMETOOLONG`. The stated goal is "capping at 4096", which implies paths of that length should be rejected.

Consider using `>=` on both guards (or keeping the constant at 4096 but treating it as an exclusive upper bound in the error message):

```suggestion
    if (includePath.length > MAX_INCLUDE_PATH_LENGTH) {
```

```suggestion
    if (normalized.length > MAX_INCLUDE_PATH_LENGTH) {
```

These two lines should be changed to `>=` to correctly exclude 4096-char paths:

```ts
    if (includePath.length >= MAX_INCLUDE_PATH_LENGTH) {
      throw new ConfigIncludeError(
        `Include path exceeds maximum length (${MAX_INCLUDE_PATH_LENGTH} characters)`,
        includePath,
      );
    }
    // ...
    if (normalized.length >= MAX_INCLUDE_PATH_LENGTH) {
      throw new ConfigIncludeError(
        `Resolved include path exceeds maximum length (${MAX_INCLUDE_PATH_LENGTH} characters)`,
        includePath,
      );
    }
```

How can I resolve this? If you propose a fix, please make it concise.

[greptile-apps]

"atLimit" case tests resolved-path limit, not raw-path limit — absolute paths at the limit are untested

"b".repeat(MAX_INCLUDE_PATH_LENGTH) (exactly 4096 bs) is a relative path, so it gets resolved to configDir + "/" + "b".repeat(4096), which is always longer than 4096 characters and is caught by the second (post-normalize) check. The first check (includePath.length > MAX_INCLUDE_PATH_LENGTH) is never exercised for boundary value 4096.

More importantly, an absolute path of exactly 4096 characters is never covered. For that case, the resolved path equals the input, normalization leaves it unchanged, and both guards evaluate to 4096 > 4096 → false — so the path passes through to the OS. Consider adding a test:

// absolute path at exactly the limit: should also be rejected
const absoluteAtLimit = "/" + "a".repeat(MAX_INCLUDE_PATH_LENGTH - 1); // length == 4096
expectResolveIncludeError(
  () => resolve({ $include: absoluteAtLimit }, {}),
  /maximum length/,
);

If the boundary is intentionally exclusive (i.e., 4096-char paths are permitted), the test name and PR description should be updated to reflect that.

Prompt To Fix With AI

This is a comment left during a code review.
Path: src/config/includes.test.ts
Line: 614-615

Comment:
**"atLimit" case tests resolved-path limit, not raw-path limit — absolute paths at the limit are untested**

`"b".repeat(MAX_INCLUDE_PATH_LENGTH)` (exactly 4096 `b`s) is a *relative* path, so it gets resolved to `configDir + "/" + "b".repeat(4096)`, which is always longer than 4096 characters and is caught by the *second* (post-normalize) check. The *first* check (`includePath.length > MAX_INCLUDE_PATH_LENGTH`) is never exercised for boundary value 4096.

More importantly, an **absolute** path of exactly 4096 characters is never covered. For that case, the resolved path equals the input, normalization leaves it unchanged, and both guards evaluate to `4096 > 4096 → false` — so the path passes through to the OS. Consider adding a test:

```ts
// absolute path at exactly the limit: should also be rejected
const absoluteAtLimit = "/" + "a".repeat(MAX_INCLUDE_PATH_LENGTH - 1); // length == 4096
expectResolveIncludeError(
  () => resolve({ $include: absoluteAtLimit }, {}),
  /maximum length/,
);
```

If the boundary is intentionally exclusive (i.e., 4096-char paths are permitted), the test name and PR description should be updated to reflect that.

How can I resolve this? If you propose a fix, please make it concise.

[maweibin]

@steipete Hi, I’ve opened a small security hardening PR for config $include resolution. It rejects paths that contain null bytes and enforces a 4096-character limit (CWE-22 defense-in-depth). All changes are covered by tests and the config docs are updated. Would be great if you could review/merge when convenient. Thanks.