feat(webui): Add file listing API; Map logs input dir into the webui service container in the Package Docker Compose project.#1688
Conversation
…service container in the Package Docker Compose project.
WalkthroughAdds TypeBox schemas for file-list requests and entries, a Fastify GET /ls endpoint that lists directory contents after filesystem checks, and a read-only host logs bind-mount for the webui service in Docker Compose. Changes
Sequence Diagram(s)mermaid Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes
Possibly related issues
Pre-merge checks and finishing touches✅ Passed checks (3 passed)
✨ Finishing touches
🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
junhaoliao
changed the title
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 5
📜 Review details
Configuration used: CodeRabbit UI
Review profile: ASSERTIVE
Plan: Pro
📒 Files selected for processing (3)
components/webui/common/src/schemas/os.ts(1 hunks)components/webui/server/src/routes/api/os/index.ts(1 hunks)tools/deployment/package/docker-compose-all.yaml(1 hunks)
🧰 Additional context used
📓 Path-based instructions (1)
**/*.{cpp,hpp,java,js,jsx,tpp,ts,tsx}
⚙️ CodeRabbit configuration file
- Prefer
false == <expression>rather than!<expression>.
Files:
components/webui/server/src/routes/api/os/index.tscomponents/webui/common/src/schemas/os.ts
🧠 Learnings (1)
📚 Learning: 2025-06-16T13:05:27.349Z
Learnt from: davemarco
Repo: y-scope/clp PR: 1015
File: components/log-viewer-webui/server/src/routes/static.ts:65-70
Timestamp: 2025-06-16T13:05:27.349Z
Learning: In components/log-viewer-webui/server/src/routes/static.ts, when decorateReply is set to true in fastifyStatic configuration, the reply.sendFile() method automatically uses the root directory configured in the static plugin registration, eliminating the need to pass the root directory as a second parameter.
Applied to files:
components/webui/server/src/routes/api/os/index.ts
🧬 Code graph analysis (2)
components/webui/server/src/routes/api/os/index.ts (1)
components/webui/common/src/schemas/os.ts (1)
FileListingSchema(27-27)
components/webui/common/src/schemas/os.ts (1)
components/webui/common/src/schemas/common.ts (1)
StringSchema(12-12)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (3)
- GitHub Check: package-image
- GitHub Check: lint-check (macos-15)
- GitHub Check: lint-check (ubuntu-24.04)
🔇 Additional comments (2)
components/webui/common/src/schemas/os.ts (1)
1-29: LGTM!The schema definitions are clean, well-documented, and follow TypeScript best practices. The type safety provided by these schemas will help ensure API consistency.
components/webui/server/src/routes/api/os/index.ts (1)
50-50: I apologize for the persistent repository access issues. Unfortunately, I'm unable to clone the repository to verify the Node.js version specification and examine the code context directly.Based on the information available, here is the rewritten review comment:
Verify Node.js version supports
dirent.parentPathbefore merging.The
parentPathproperty onfs.Direntwas added in Node.js v20.9.0. Confirm that the project's Node.js version (check Dockerfile, package.jsonenginesfield, or .nvmrc) is >= 20.9.0. If targeting earlier Node.js versions, construct the parent path manually using therequestedPathvariable:const isExpandable = dirent.isDirectory() || dirent.isSymbolicLink(); return { isExpandable: isExpandable, name: dirent.name, - parentPath: dirent.parentPath, + parentPath: requestedPath, };
| const FileListRequestSchema = Type.Object({ | ||
| path: Type.String({ | ||
| default: "/", | ||
| }), | ||
| }); |
There was a problem hiding this comment.
Critical: Add path validation to prevent path traversal attacks.
The schema accepts any string path without validation, allowing potential path traversal attacks (e.g., ?path=../../../etc/passwd). The default root path "/" also grants access to the entire container filesystem.
Recommendations:
- Validate that the requested path is within the allowed directory (e.g.,
/mnt/logs) - Sanitize the path to remove
..segments - Consider using
path.resolve()and checking if the resolved path starts with the allowed base path
Example validation:
import path from "path";
const ALLOWED_BASE_PATH = process.env.CLP_LOGS_INPUT_DIR_CONTAINER || "/mnt/logs";
const FileListRequestSchema = Type.Object({
path: Type.String({
default: ALLOWED_BASE_PATH,
}),
});
// In the handler, add validation:
const resolvedPath = path.resolve(requestedPath);
if (!resolvedPath.startsWith(path.resolve(ALLOWED_BASE_PATH))) {
return await reply.forbidden("Access denied: path outside allowed directory");
}🤖 Prompt for AI Agents
In components/webui/server/src/routes/api/os/index.ts around lines 8-12, the
FileListRequestSchema accepts any string and uses "/" by default which allows
path traversal and access to the entire container; change the schema default to
an allowed base (e.g., process.env.CLP_LOGS_INPUT_DIR_CONTAINER || "/mnt/logs"),
import path, and in the route handler sanitize and resolve the requested path
(use path.normalize/path.resolve to collapse '..' segments) then verify the
resolved path starts with the resolved allowed base path; if the check fails
return a forbidden response. Ensure you do not change client-visible API shape
beyond the default and keep the allowed base configurable via env.
There was a problem hiding this comment.
at the moment, no sensitive information is mapped into the container, and path filtering is not planned
There was a problem hiding this comment.
@junhaoliao, understood! I acknowledge that the security model for this file listing API relies on container isolation with controlled volume mappings rather than explicit path validation within the application code. Since only specific directories are mounted into the container, the Docker isolation provides the security boundary.
Thank you for clarifying the architectural decision—I've already noted this for future reviews.
🧠 Learnings used
Learnt from: junhaoliao
Repo: y-scope/clp PR: 1688
File: components/webui/server/src/routes/api/os/index.ts:46-46
Timestamp: 2025-11-29T08:03:34.246Z
Learning: In the WebUI server (components/webui/server), the file listing API relies on container isolation for security rather than explicit path validation like ALLOWED_BASE_PATH. The service runs in a Docker container with controlled volume mappings, which provides the security boundary.
Learnt from: davemarco
Repo: y-scope/clp PR: 1015
File: components/log-viewer-webui/server/src/routes/static.ts:65-70
Timestamp: 2025-06-16T13:05:27.349Z
Learning: In components/log-viewer-webui/server/src/routes/static.ts, when decorateReply is set to true in fastifyStatic configuration, the reply.sendFile() method automatically uses the root directory configured in the static plugin registration, eliminating the need to pass the root directory as a second parameter.
| try { | ||
| await fs.access(requestedPath); | ||
| } catch { | ||
| return await reply.notFound(`Path not found: ${requestedPath}`); | ||
| } |
There was a problem hiding this comment.
Major: Verify the path is a directory, not just that it exists.
The current implementation uses fs.access() which only checks if the path exists and is accessible. A path could be a regular file, causing fs.readdir() to fail on line 44.
Apply this diff to add directory validation:
try {
- await fs.access(requestedPath);
+ const stats = await fs.stat(requestedPath);
+ if (!stats.isDirectory()) {
+ return await reply.badRequest(`Path is not a directory: ${requestedPath}`);
+ }
} catch {
return await reply.notFound(`Path not found: ${requestedPath}`);
}📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| try { | |
| await fs.access(requestedPath); | |
| } catch { | |
| return await reply.notFound(`Path not found: ${requestedPath}`); | |
| } | |
| try { | |
| const stats = await fs.stat(requestedPath); | |
| if (false == stats.isDirectory()) { | |
| return await reply.badRequest(`Path is not a directory: ${requestedPath}`); | |
| } | |
| } catch { | |
| return await reply.notFound(`Path not found: ${requestedPath}`); | |
| } |
🤖 Prompt for AI Agents
In components/webui/server/src/routes/api/os/index.ts around lines 37 to 41, the
code currently uses fs.access() which only verifies existence; change it to stat
the path (using await fs.stat(requestedPath) or fs.lstat) and check
stat.isDirectory() before continuing, and if the stat call fails or
isDirectory() is false return reply.notFound(`Path not found: ${requestedPath}`)
(or an appropriate error); this ensures you only call fs.readdir() on actual
directories.
There was a problem hiding this comment.
don't we also need to check for symlinks?
we only list "expandable" entries in the client; also, any failures erroneous user actions would be caught in the catch and handled, right?
There was a problem hiding this comment.
@junhaoliao Good questions! Let me address both:
Symlinks: No special handling needed. fs.stat() (as opposed to fs.lstat()) follows symlinks by default, so a symlink pointing to a directory will correctly pass the isDirectory() check. This means symlinks to directories will work as expected and can be listed.
Error handling: You're right that errors would be caught, but there's a subtle difference in user experience:
-
Current approach (
fs.access()): If a user requests a regular file path,fs.readdir()will fail and the catch block returns a 500 (Internal Server Error) with a generic message. This makes it look like a server problem rather than a client mistake. -
Suggested approach (
fs.stat()+isDirectory()): Returns a 400 (Bad Request) with a clear message "Path is not a directory", which more accurately indicates it's a client error.
Both approaches work functionally, but the explicit directory check provides clearer feedback with appropriate HTTP status codes. That said, if you prefer to keep the current implementation since errors are handled, that's a valid choice—it's more of a UX improvement than a critical fix.
🧠 Learnings used
Learnt from: junhaoliao
Repo: y-scope/clp PR: 1688
File: components/webui/server/src/routes/api/os/index.ts:46-46
Timestamp: 2025-11-29T08:03:34.246Z
Learning: In the WebUI server (components/webui/server), the file listing API relies on container isolation for security rather than explicit path validation like ALLOWED_BASE_PATH. The service runs in a Docker container with controlled volume mappings, which provides the security boundary.
Learnt from: davemarco
Repo: y-scope/clp PR: 1015
File: components/log-viewer-webui/server/src/routes/static.ts:65-70
Timestamp: 2025-06-16T13:05:27.349Z
Learning: In components/log-viewer-webui/server/src/routes/static.ts, when decorateReply is set to true in fastifyStatic configuration, the reply.sendFile() method automatically uses the root directory configured in the static plugin registration, eliminating the need to pass the root directory as a second parameter.
| try { | ||
| const direntList = await fs.readdir(requestedPath, {withFileTypes: true}); | ||
| return direntList.map((dirent) => { | ||
| const isExpandable = dirent.isDirectory() || dirent.isSymbolicLink(); | ||
| return { | ||
| isExpandable: isExpandable, | ||
| name: dirent.name, | ||
| parentPath: dirent.parentPath, | ||
| }; | ||
| }); |
There was a problem hiding this comment.
Major: Add pagination or entry limit to prevent DoS with large directories.
Reading all directory entries without limits could cause memory exhaustion or timeouts when listing directories with thousands or millions of files.
Consider adding:
- A maximum entry limit in the request schema
- Pagination support with offset/limit parameters
- Streaming response for very large directories
Example with limit:
const FileListRequestSchema = Type.Object({
path: Type.String({
default: "/",
}),
limit: Type.Optional(Type.Number({
minimum: 1,
maximum: 1000,
default: 100,
})),
});
// In handler:
const {path: requestedPath, limit = 100} = request.query;
const direntList = await fs.readdir(requestedPath, {withFileTypes: true});
return direntList.slice(0, limit).map((dirent) => {
// ... mapping logic
});🤖 Prompt for AI Agents
In components/webui/server/src/routes/api/os/index.ts around lines 43 to 52, the
handler reads all directory entries which can cause memory/time DoS for very
large directories — add a safe entry limit and optional pagination: extend the
request schema to accept limit (number, min 1, max 1000, default 100) and
optional offset/page parameters, read the full dirents only if necessary or use
a streaming/iterative read if available, then apply slice(offset, offset +
limit) before mapping and returning results; validate and coerce query values,
and ensure the default limit is applied when none provided.
There was a problem hiding this comment.
There might be data inconsistency if pagination isn't properly implemented. That you are using numerical offsets as cursors would cause missing / repeated listed entries when files are removed / added in between requests. A more proper implementation would be to sort the paths alphabetically and use the last listed path as cursors, though we still need to properly handle added / removed files.
i think it's better to defer this
There was a problem hiding this comment.
@junhaoliao You're absolutely right about the cursor-based pagination complexity—using alphabetically sorted paths as cursors is indeed more robust than numerical offsets when dealing with filesystem changes between requests.
I understand the decision to defer this feature. Would you like me to create a follow-up issue to track this work for a future PR? I can document the pagination requirements and the cursor-based approach you mentioned.
🧠 Learnings used
Learnt from: junhaoliao
Repo: y-scope/clp PR: 1688
File: components/webui/server/src/routes/api/os/index.ts:46-46
Timestamp: 2025-11-29T08:03:34.246Z
Learning: In the WebUI server (components/webui/server), the file listing API relies on container isolation for security rather than explicit path validation like ALLOWED_BASE_PATH. The service runs in a Docker container with controlled volume mappings, which provides the security boundary.
Learnt from: davemarco
Repo: y-scope/clp PR: 1015
File: components/log-viewer-webui/server/src/routes/static.ts:65-70
Timestamp: 2025-06-16T13:05:27.349Z
Learning: In components/log-viewer-webui/server/src/routes/static.ts, when decorateReply is set to true in fastifyStatic configuration, the reply.sendFile() method automatically uses the root directory configured in the static plugin registration, eliminating the need to pass the root directory as a second parameter.
| try { | ||
| const direntList = await fs.readdir(requestedPath, {withFileTypes: true}); | ||
| return direntList.map((dirent) => { | ||
| const isExpandable = dirent.isDirectory() || dirent.isSymbolicLink(); |
There was a problem hiding this comment.
Minor: Symbolic link resolution could lead to security issues.
Marking symbolic links as expandable without resolving them first could expose paths outside the allowed directory if symlinks point to sensitive locations.
Consider either:
- Resolving symlinks and checking if they point within the allowed directory
- Not marking symlinks as expandable
- Adding a flag to control symlink behavior
Example resolution check:
let isExpandable = dirent.isDirectory();
if (dirent.isSymbolicLink()) {
try {
const symlinkPath = path.join(requestedPath, dirent.name);
const realPath = await fs.realpath(symlinkPath);
const resolvedStats = await fs.stat(realPath);
isExpandable = resolvedStats.isDirectory() &&
realPath.startsWith(ALLOWED_BASE_PATH);
} catch {
isExpandable = false;
}
}🤖 Prompt for AI Agents
In components/webui/server/src/routes/api/os/index.ts around line 46, currently
symlinks are treated as expandable which can expose paths outside the allowed
directory; update the logic to resolve symbolic links before marking expandable:
when dirent.isSymbolicLink() use fs.realpath on the joined entry path, stat the
resolved path, set isExpandable only if the resolved path is a directory and its
realPath startsWith the ALLOWED_BASE_PATH (catching errors and treating as
non-expandable); alternatively, if you prefer simpler behavior, do not mark
symlinks as expandable or add a configuration flag (e.g., allowSymlinks) to
explicitly control this behavior and ensure all branches fall back to
non-expandable on any error.
There was a problem hiding this comment.
The service runs in a container, so data on the host is relatively safe unless fully mapped. There is no planned support for ALLOWED_BASE_PATH yet.
There was a problem hiding this comment.
@junhaoliao, understood! That makes sense—the container isolation does provide a security boundary, and as long as the volume mappings are carefully controlled, the symlink behavior should be manageable within that context.
If you do expand the filesystem exposure in future updates, it may be worth revisiting symlink handling, but for the current scope this approach is reasonable.
✏️ Learnings added
Learnt from: junhaoliao
Repo: y-scope/clp PR: 1688
File: components/webui/server/src/routes/api/os/index.ts:46-46
Timestamp: 2025-11-29T08:03:34.246Z
Learning: In the WebUI server (components/webui/server), the file listing API relies on container isolation for security rather than explicit path validation like ALLOWED_BASE_PATH. The service runs in a Docker container with controlled volume mappings, which provides the security boundary.
Note: Learnings are effective only in the context of similar code segments. To apply general coding guidelines, please configure review instructions. You can manage existing learnings in the app.
🧠 Learnings used
Learnt from: davemarco
Repo: y-scope/clp PR: 1015
File: components/log-viewer-webui/server/src/routes/static.ts:65-70
Timestamp: 2025-06-16T13:05:27.349Z
Learning: In components/log-viewer-webui/server/src/routes/static.ts, when decorateReply is set to true in fastifyStatic configuration, the reply.sendFile() method automatically uses the root directory configured in the static plugin registration, eliminating the need to pass the root directory as a second parameter.
There was a problem hiding this comment.
all "${CLP_LOGS_INPUT_DIR_HOST:-empty}:${CLP_LOGS_INPUT_DIR_CONTAINER:-/mnt/logs}" should be made readonly in this file
@coderabbitai file a bug report
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Find all occurrences of the CLP_LOGS_INPUT_DIR mount pattern
echo "=== Finding all occurrences of CLP_LOGS_INPUT_DIR mount pattern ==="
rg -n 'CLP_LOGS_INPUT_DIR_HOST.*CLP_LOGS_INPUT_DIR_CONTAINER' tools/deployment/package/docker-compose-all.yaml
Length of output: 340
🏁 Script executed:
#!/bin/bash
# Create a GitHub issue for making the logs input directory mounts readonly
gh issue create \
--repo y-scope/clp \
--title "Make CLP_LOGS_INPUT_DIR mounts readonly in docker-compose-all.yaml" \
--body "## Description
All mount points for \`CLP_LOGS_INPUT_DIR_HOST\` in \`tools/deployment/package/docker-compose-all.yaml\` should be made readonly by adding the \`:ro\` suffix.
## Affected Services
Currently, the following services mount the logs input directory without the readonly flag:
- \`compression-scheduler\`
- \`compression-worker\`
- \`webui\`
## Expected Behavior
The mount should be:
\`\`\`yaml
\"\${CLP_LOGS_INPUT_DIR_HOST:-empty}:\${CLP_LOGS_INPUT_DIR_CONTAINER:-/mnt/logs}:ro\"
\`\`\`
## Context
This issue was identified in PR #1688: https://github.com/y-scope/clp/pull/1688#discussion_r2572350167
Requested by: @junhaoliao" \
--assignee junhaoliao \
--label bug
Length of output: 42
@junhaoliao, I've filed a bug report for this issue!
Issue created: #1689
I found 3 occurrences of the CLP_LOGS_INPUT_DIR mount pattern in docker-compose-all.yaml that need to be made readonly:
- Line 237 -
compression-schedulerservice - Line 271 -
compression-workerservice - Line 308 -
webuiservice
All of these should have the :ro suffix added to ensure the logs input directory is mounted as readonly, preventing any accidental writes to the input logs.
🧠 Learnings used
Learnt from: junhaoliao
Repo: y-scope/clp PR: 1178
File: components/clp-py-utils/clp_py_utils/clp_config.py:47-47
Timestamp: 2025-10-07T07:54:32.427Z
Learning: In components/clp-py-utils/clp_py_utils/clp_config.py, the CONTAINER_AWS_CONFIG_DIRECTORY constant is intentionally set to pathlib.Path("/") / ".aws" (i.e., `/.aws`) rather than a user-specific home directory. This hardcoded path is part of the container orchestration design.
Learnt from: junhaoliao
Repo: y-scope/clp PR: 1152
File: components/clp-package-utils/clp_package_utils/general.py:0-0
Timestamp: 2025-08-19T14:41:28.901Z
Learning: In the CLP codebase, prefer explicit failures over automatic directory creation in utility functions like dump_config. The user junhaoliao prefers to let file operations fail when parent directories don't exist, as this helps catch implementation errors during development rather than masking setup issues with automatic directory creation.
Learnt from: jackluo923
Repo: y-scope/clp PR: 1054
File: components/core/tools/scripts/lib_install/musllinux_1_2/install-packages-from-source.sh:6-8
Timestamp: 2025-07-01T14:51:19.172Z
Learning: In CLP installation scripts within `components/core/tools/scripts/lib_install/`, maintain consistency with existing variable declaration patterns across platforms rather than adding individual improvements like `readonly` declarations.
Learnt from: jackluo923
Repo: y-scope/clp PR: 1054
File: components/core/tools/scripts/lib_install/musllinux_1_2/install-packages-from-source.sh:6-8
Timestamp: 2025-07-01T14:51:19.172Z
Learning: In CLP installation scripts within `components/core/tools/scripts/lib_install/`, maintain consistency with existing variable declaration patterns across platforms rather than adding individual improvements like `readonly` declarations.
Learnt from: junhaoliao
Repo: y-scope/clp PR: 1414
File: tools/docker-images/clp-package/Dockerfile:20-24
Timestamp: 2025-10-13T03:32:19.293Z
Learning: In the clp repository's Dockerfiles (e.g., tools/docker-images/clp-package/Dockerfile), ENV directives should be split into separate lines for readability rather than consolidated to reduce layer count. This is especially true for PATH modifications, as agreed upon in PR #1166. Later ENV settings may depend on earlier ones (e.g., referencing CLP_HOME).
…ocker Compose configuration
| "/ls", | ||
| { | ||
| schema: { | ||
| querystring: FileListRequestSchema, |
There was a problem hiding this comment.
shouldnt should just be body instead of querystring?
There was a problem hiding this comment.
the requests look like
GET http://localhost:4000/os/ls?path=/mnt/logs/home/junhao/samples
so querystring should be a better schema field we want to set?
There was a problem hiding this comment.
ignore i didnt realize was get
| schema: { | ||
| querystring: FileListRequestSchema, | ||
| response: { | ||
| [constants.HTTP_STATUS_OK]: FileListingSchema, |
There was a problem hiding this comment.
Should we include the tag like the other routes
| }, | ||
| }, | ||
| async (request, reply) => { | ||
| const {path: requestedPath} = request.query; |
There was a problem hiding this comment.
once you add the body, i think u can get the path easier from the body field
There was a problem hiding this comment.
ah i see what you mean. the request is a GET request, and it's unconventional for GET requests to include a body (technically we can, but most client libraries / JS don't handle GET requests with a body well
There was a problem hiding this comment.
ah i see, sorry i didnt notice was get since most of the ones i did were post. ignore this comment
| import {constants} from "http2"; | ||
|
|
||
|
|
||
| const FileListRequestSchema = Type.Object({ |
There was a problem hiding this comment.
I think you should put this in the schemas folder.
There was a problem hiding this comment.
Actionable comments posted: 1
♻️ Duplicate comments (1)
components/webui/server/src/routes/api/os/index.ts (1)
33-41: Consider validating thatrequestedPathis a directory before listingRight now you only check accessibility with
fs.access. If a client (or a race) pointsrequestedPathat a regular file,fs.readdirwill throw (e.g.,ENOTDIR), and the handler responds with a 500 “Unable to list files”, which looks like a server fault rather than a client/path issue.A small
fs.statcheck lets you return a clearer 4xx response for “not a directory” while keeping the existing 404 behaviour for missing paths.Example:
- try { - await fs.access(requestedPath); - } catch { - return await reply.notFound(`Path not found: ${requestedPath}`); - } + try { + const stats = await fs.stat(requestedPath); + if (false == stats.isDirectory()) { + return await reply.badRequest(`Path is not a directory: ${requestedPath}`); + } + } catch { + return await reply.notFound(`Path not found: ${requestedPath}`); + }If you want to re-confirm the
fs.stat().isDirectory()behaviour and ENOTDIR conditions:In Node.js, how does `fs.readdir` behave when given a path to a regular file, and is `fs.stat(...).isDirectory()` the idiomatic way to check for directories before calling `fs.readdir`?
📜 Review details
Configuration used: CodeRabbit UI
Review profile: ASSERTIVE
Plan: Pro
📒 Files selected for processing (1)
components/webui/server/src/routes/api/os/index.ts(1 hunks)
🧰 Additional context used
📓 Path-based instructions (1)
**/*.{cpp,hpp,java,js,jsx,tpp,ts,tsx}
⚙️ CodeRabbit configuration file
- Prefer
false == <expression>rather than!<expression>.
Files:
components/webui/server/src/routes/api/os/index.ts
🧠 Learnings (4)
📓 Common learnings
Learnt from: junhaoliao
Repo: y-scope/clp PR: 1688
File: components/webui/server/src/routes/api/os/index.ts:46-46
Timestamp: 2025-11-29T08:03:34.272Z
Learning: In the WebUI server (components/webui/server), the file listing API relies on container isolation for security rather than explicit path validation like ALLOWED_BASE_PATH. The service runs in a Docker container with controlled volume mappings, which provides the security boundary.
📚 Learning: 2025-06-16T13:05:27.349Z
Learnt from: davemarco
Repo: y-scope/clp PR: 1015
File: components/log-viewer-webui/server/src/routes/static.ts:65-70
Timestamp: 2025-06-16T13:05:27.349Z
Learning: In components/log-viewer-webui/server/src/routes/static.ts, when decorateReply is set to true in fastifyStatic configuration, the reply.sendFile() method automatically uses the root directory configured in the static plugin registration, eliminating the need to pass the root directory as a second parameter.
Applied to files:
components/webui/server/src/routes/api/os/index.ts
📚 Learning: 2025-11-29T08:03:34.272Z
Learnt from: junhaoliao
Repo: y-scope/clp PR: 1688
File: components/webui/server/src/routes/api/os/index.ts:46-46
Timestamp: 2025-11-29T08:03:34.272Z
Learning: In the WebUI server (components/webui/server), the file listing API relies on container isolation for security rather than explicit path validation like ALLOWED_BASE_PATH. The service runs in a Docker container with controlled volume mappings, which provides the security boundary.
Applied to files:
components/webui/server/src/routes/api/os/index.ts
📚 Learning: 2025-07-29T21:00:07.757Z
Learnt from: junhaoliao
Repo: y-scope/clp PR: 1136
File: components/webui/client/src/pages/IngestPage/Details/sql.ts:1-1
Timestamp: 2025-07-29T21:00:07.757Z
Learning: User junhaoliao requested creating a GitHub issue to track server-side SQL query error handling improvements, specifically to prevent uncaught query errors from causing 500 errors to reach the client.
Applied to files:
components/webui/server/src/routes/api/os/index.ts
🧬 Code graph analysis (1)
components/webui/server/src/routes/api/os/index.ts (1)
components/webui/common/src/schemas/os.ts (2)
FileListRequestSchema(34-34)FileListingSchema(33-33)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (5)
- GitHub Check: package-image
- GitHub Check: lint-check (ubuntu-24.04)
- GitHub Check: lint-check (macos-15)
- GitHub Check: build (macos-15)
- GitHub Check: build (ubuntu-24.04)
| return direntList.map((dirent) => { | ||
| const isExpandable = dirent.isDirectory() || dirent.isSymbolicLink(); | ||
| return { | ||
| isExpandable: isExpandable, | ||
| name: dirent.name, | ||
| parentPath: dirent.parentPath, | ||
| }; |
There was a problem hiding this comment.
🧩 Analysis chain
🌐 Web query:
Node.js fs.Dirent properties withFileTypes readdir API
💡 Result:
Summary — fs.Dirent and readdir(withFileTypes)
-
fs.readdir(path, { withFileTypes: true }, ...) returns an array of fs.Dirent objects (instead of name strings). Use the synchronous or async variants the same way. [1][4]
-
fs.Dirent exposes:
- property: name (file name). [1]
- methods that return boolean: isFile(), isDirectory(), isSymbolicLink(), isBlockDevice(), isCharacterDevice(), isFIFO(), isSocket(). Use these to test the entry type without additional stat calls. [1][2]
-
Implementation note: Node uses an internal Symbol-backed field to store the libuv dirent type (UV_DIRENT_*); that internal property is not part of the public API — rely on the documented isX() methods. [3]
Minimal example (async):
const fs = require('fs/promises');
const entries = await fs.readdir('./dir', { withFileTypes: true });
for (const d of entries) {
console.log(d.name, d.isDirectory() ? 'dir' : d.isFile() ? 'file' : 'other');
}
References
[1] Node.js fs (Dirent + readdir docs).
[2] Node.js fs.Dirent docs (method list).
[3] Explanation of internal Symbol("type") / libuv dirent types.
[4] fs.readdir(withFileTypes) usage description.
🏁 Script executed:
cat -n components/webui/server/src/routes/api/os/index.tsRepository: y-scope/clp
Length of output: 166
🏁 Script executed:
git ls-files components/webui/server/src/routes/api/os/index.tsRepository: y-scope/clp
Length of output: 166
🏁 Script executed:
git show HEAD:components/webui/server/src/routes/api/os/index.ts 2>/dev/null | head -80Repository: y-scope/clp
Length of output: 166
Fix invalid parentPath source when mapping directory entries
fs.readdir(..., {withFileTypes: true}) returns Dirent objects with only a name property and type-check methods (isDirectory(), isSymbolicLink(), etc.). The parentPath property does not exist on Dirent and will cause a TypeScript compilation error (property does not exist on type 'Dirent').
Use requestedPath as the parent directory instead:
return direntList.map((dirent) => {
const isExpandable = dirent.isDirectory() || dirent.isSymbolicLink();
return {
isExpandable: isExpandable,
name: dirent.name,
- parentPath: dirent.parentPath,
+ parentPath: requestedPath,
};
});📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| return direntList.map((dirent) => { | |
| const isExpandable = dirent.isDirectory() || dirent.isSymbolicLink(); | |
| return { | |
| isExpandable: isExpandable, | |
| name: dirent.name, | |
| parentPath: dirent.parentPath, | |
| }; | |
| return direntList.map((dirent) => { | |
| const isExpandable = dirent.isDirectory() || dirent.isSymbolicLink(); | |
| return { | |
| isExpandable: isExpandable, | |
| name: dirent.name, | |
| parentPath: requestedPath, | |
| }; |
🤖 Prompt for AI Agents
In components/webui/server/src/routes/api/os/index.ts around lines 41 to 47, the
mapping uses dirent.parentPath which does not exist on fs.Dirent and causes a
TypeScript error; replace the non-existent property with the requestedPath (the
directory that was read) when constructing the returned object and keep the
isExpandable and name properties as-is so the parentPath points to the actual
directory requested.
davemarco
left a comment
davemarco
left a comment
There was a problem hiding this comment.
lgtm but maybe wait till after release to merge just in case
junhaoliao
changed the title
2 participants
for integration, refer to #1292
Description
This PR introduces a new file listing API and map logs input dir into the webui service container in the Package Docker Compose project to allow listing log input files. Changes include:
FileEntrySchemaandFileListingSchemaincomponents/webui/common/src/schemas/os.tsfor consistent typing of file system entries./lsendpoint incomponents/webui/server/src/routes/api/os/index.tsto list files and directories at a specified path with proper validation and error handling.docker-compose-all.yamlto mount the host logs input directory to the container for API access.Checklist
breaking change.
Validation performed
docker-compose-all.yamlmounting logs input directory./api/os/lsendpoint:Summary by CodeRabbit
New Features
Chores
✏️ Tip: You can customize this high-level summary in your review settings.