C API: add nix_locked_flake_read_path for flake file reading #244
Conversation
WalkthroughAdds a new C API function Changes
Sequence Diagram(s)sequenceDiagram
participant Caller as C caller
participant CAPI as libflake-c API
participant Flake as LockedFlake / SourcePath
rect rgba(135,206,250,0.12)
Note over CAPI,Flake: New flow: read file relative to locked flake parent
end
Caller->>CAPI: nix_locked_flake_read_path(ctx, lockedFlake, path, cb, ud)
CAPI->>CAPI: clear context errors
CAPI->>Flake: resolve parent + append path
CAPI->>Flake: readFile()
alt read success
Flake-->>CAPI: contents (string)
CAPI-->>Caller: invoke cb(contents, ud)
CAPI-->>Caller: return NIX_OK
else read error / exception
Flake-->>CAPI: throw / error
CAPI->>CAPI: map to nix_err via NIXC_CATCH_ERRS
CAPI-->>Caller: return error code
end
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes
Suggested reviewers
Poem
Pre-merge checks and finishing touches✅ Passed checks (2 passed)
✨ Finishing touches
🧪 Generate unit tests (beta)
📜 Recent review detailsConfiguration used: CodeRabbit UI Review profile: CHILL Plan: Pro 📒 Files selected for processing (2)
🚧 Files skipped from review as they are similar to previous changes (2)
⏰ 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). (4)
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 |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
src/libflake-c/nix_api_flake.cc(1 hunks)src/libflake-c/nix_api_flake.h(1 hunks)
🧰 Additional context used
🧬 Code graph analysis (2)
src/libflake-c/nix_api_flake.h (1)
src/libflake-c/nix_api_flake.cc (2)
nix_locked_flake_get_path(209-218)nix_locked_flake_get_path(209-210)
src/libflake-c/nix_api_flake.cc (1)
src/libutil-c/nix_api_util.cc (4)
nix_clear_err(64-68)nix_clear_err(64-64)call_nix_get_string_callback(156-160)call_nix_get_string_callback(156-156)
⏰ 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: build_x86_64-linux / vm_tests_smoke
- GitHub Check: build_x86_64-linux / test
- GitHub Check: build_x86_64-linux / manual
- GitHub Check: build_aarch64-darwin / build
- GitHub Check: eval
🔇 Additional comments (1)
src/libflake-c/nix_api_flake.cc (1)
209-218: Verify the.parent()call aligns with the intended API behavior.The implementation calls
.parent()on line 214, returning the parent directory of the flake path rather than the flake path itself. Given:
- The function is named
nix_locked_flake_get_path- The PR objective states: "obtain the path of a Flake when the Flake is not local"
Please confirm whether returning the parent directory is intentional. If users need the actual flake path (e.g.,
/path/to/flakeinstead of/path/to), consider removing the.parent()call:- auto v = lockedFlake->lockedFlake->flake.path.parent().to_string(); + auto v = lockedFlake->lockedFlake->flake.path.to_string();If the parent directory is indeed the intended return value, update the function documentation in the header file accordingly.
| /** | ||
| * @brief Get the root path of a flake. | ||
| * @note The callback borrows the string only for the duration of the call. | ||
| * | ||
| * @param[out] context Optional, stores error information | ||
| * @param[in] locked_flake the flake to get the path for | ||
| * @param[in] callback The callback to call with the string | ||
| * @param[in] user_data Additional data to pass for the callback | ||
| */ | ||
| nix_err nix_locked_flake_get_path( | ||
| nix_c_context * context, nix_locked_flake * lockedFlake, nix_get_string_callback callback, void * user_data); | ||
|
|
There was a problem hiding this comment.
Clarify documentation: "root path" vs "parent directory".
The documentation states "Get the root path of a flake," but the implementation at line 214 in src/libflake-c/nix_api_flake.cc calls .parent(), which returns the parent directory of the flake path, not the flake path itself.
Please clarify the documentation to accurately reflect what the function returns. If the intent is to return the directory containing the flake, consider updating the doc comment to:
/**
- * @brief Get the root path of a flake.
+ * @brief Get the parent directory path of a flake.
* @note The callback borrows the string only for the duration of the call.Alternatively, if the function should return the flake path itself (not the parent), remove the .parent() call in the implementation.
🤖 Prompt for AI Agents
In src/libflake-c/nix_api_flake.h around lines 241-252, the doc comment
incorrectly says "root path of a flake" while the implementation returns the
parent directory; update the comment to state it returns the parent directory
containing the flake (e.g., "Get the parent directory containing the flake") and
clarify the borrowing semantics, or if the intended behavior is to return the
flake path itself, instead modify the implementation to return the flake path
(remove the .parent() call) and adjust tests/usage accordingly so the header and
implementation match.
edolstra
changed the title
src/libflake-c/nix_api_flake.h
Outdated
| nix_c_context * context, nix_flake_settings * settings, EvalState * evalState, nix_locked_flake * lockedFlake); | ||
|
|
||
| /** | ||
| * @brief Get the root path of a flake. |
There was a problem hiding this comment.
Note that when lazy trees are enabled, flake.path is a virtual (i.e. random) path that doesn't actually exist. So this is probably not a useful thing to expose.
Depending on the intended use, you could:
- Copy the path to the Nix store by calling
state.devirtualize(). This can be expensive. - Create C bindings for the
SourceAccessor/SourcePathAPI to allow the caller to access the flake lazily.
There was a problem hiding this comment.
Hmm, that's a good point. Is there a way to detect when lazy trees is active? I know I somewhat need a real file since I'm calling std::fs::read_to_string() within rust.
There was a problem hiding this comment.
Might be better then to provide an API to return the contents of a file in a flake (i.e. wrapping SourcePath::readFile()). Then it works for the lazy and non-lazy case. E.g. to read flake.lock you would do
nix_locked_flake_get_path(context, lockedFlake, "/flake.lock", callback, ...);
where callback is called with the contents of the file.
There was a problem hiding this comment.
Hmm, that can work
RossComputerGuy
force-pushed
the
RossComputerGuy/flake-path-capi
branch
from
641c031 to
23fae53
Compare
RossComputerGuy
changed the title
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Nitpick comments (1)
src/libflake-c/nix_api_flake.cc (1)
209-223: Add documentation for this new API function.The function lacks documentation explaining:
- Its purpose (read file contents relative to the flake's parent directory)
- The expected format/constraints of the
pathparameter- Why
parent()is used instead of the flake path directly- Example usage or common use cases
Adding inline comments or header documentation would improve clarity for API consumers.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
src/libflake-c/nix_api_flake.cc(1 hunks)src/libflake-c/nix_api_flake.h(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- src/libflake-c/nix_api_flake.h
🧰 Additional context used
🧬 Code graph analysis (1)
src/libflake-c/nix_api_flake.cc (1)
src/libutil-c/nix_api_util.cc (4)
nix_clear_err(64-68)nix_clear_err(64-64)call_nix_get_string_callback(156-160)call_nix_get_string_callback(156-156)
⏰ 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). (1)
- GitHub Check: build_aarch64-darwin / build
| nix_err nix_locked_flake_read_path( | ||
| nix_c_context * context, | ||
| nix_locked_flake * lockedFlake, | ||
| const char * path, | ||
| nix_get_string_callback callback, | ||
| void * user_data) | ||
| { | ||
| nix_clear_err(context); | ||
| try { | ||
| auto source_path = lockedFlake->lockedFlake->flake.path.parent() / path; | ||
| auto v = source_path.readFile(); | ||
| return call_nix_get_string_callback(v, callback, user_data); | ||
| } | ||
| NIXC_CATCH_ERRS | ||
| } |
There was a problem hiding this comment.
Clarify the function's purpose—PR title/motivation vs. implementation mismatch.
The PR title mentions "nix_locked_flake_get_path" and the motivation states "obtain the path of a Flake," but this implementation is named "nix_locked_flake_read_path" and reads file contents rather than returning a path. This creates confusion about the intended API:
- If the goal is to get the flake's path, the function should return/callback with the path string itself, not read a file.
- If the goal is to read a file relative to the flake, then the PR title and motivation need updating to match.
Please clarify which behavior is intended and ensure the PR description, function name, and implementation are aligned.
🤖 Prompt for AI Agents
In src/libflake-c/nix_api_flake.cc around lines 209-223, the function name and
implementation disagree with the PR title/motivation: either rename the function
to reflect that it reads file contents (e.g.,
nix_locked_flake_read_file/read_path_file) and update the PR title/description
and API docs to state it returns file contents, or change the implementation so
it returns the flake path string (call the callback with
lockedFlake->lockedFlake->flake.path.string()) and rename it to
nix_locked_flake_get_path; update related header declarations, tests, and any
callers to match the chosen behavior so name, docs, and implementation are
consistent.
src/libflake-c/nix_api_flake.cc
Outdated
| { | ||
| nix_clear_err(context); | ||
| try { | ||
| auto source_path = lockedFlake->lockedFlake->flake.path.parent() / path; |
There was a problem hiding this comment.
🧩 Analysis chain
Address path traversal risk and clarify parent() usage.
Line 218 constructs the file path without validating the path parameter, which could allow path traversal attacks if callers pass values like "../../etc/passwd". Additionally, using .parent() on the flake path is unclear:
- Why resolve relative to the parent directory instead of the flake directory itself?
- What is the expected use case for this behavior?
Consider:
- Validating/sanitizing the
pathparameter to prevent traversal outside the intended directory. - Documenting why
parent()is used, or usinglockedFlake->lockedFlake->flake.pathdirectly if that's the intended base.
To verify if path validation/canonicalization utilities exist in the codebase that could be applied here:
🏁 Script executed:
#!/bin/bash
# Search for path validation or canonicalization patterns
rg -nP --type=cpp -C3 '\b(canonPath|realPath|isInDir|sanitize.*[Pp]ath)\s*\('Length of output: 18113
I need to examine the context around line 218 in the file to understand the parameter flow and usage pattern:
Now I need to examine the specific context around line 218 in nix_api_flake.cc:
I need to examine the actual nix_api_flake.cc file to understand the function context. Let me run a shell script:
This search will help me locate the exact function and context. However, since I don't have direct execution capability, let me try a different approach by examining if the file and function can be found through web search specific to the codebase issue:
Based on my analysis of the shell script output from the initial search and the web research, I can now provide my findings:
Based on my comprehensive verification:
Confirm path traversal vulnerability concern and recommend validation.
Path validation utilities (canonPath, isInDir, isDirOrInDir) are confirmed available in src/libutil/include/nix/util/file-system.hh and actively used throughout the codebase. The pattern of validating paths before use exists in src/libstore/unix/build/derivation-builder.cc (lines 878-886), where paths are canonicalized and checked against allowlists using isDirOrInDir().
Your concern about line 218 is valid—without validation of the path parameter, a caller could pass values like "../../etc/passwd" to traverse outside the intended directory. The recommended approach is to employ weakly_canonical() or canonPath() to normalize the path and then check that the result is prefixed with your base path using isInDir().
The use of .parent() on the locked flake path is also worth clarifying in a comment—document why the parent directory is the intended base instead of the flake directory itself.
🏁 Script executed:
[run_scripts]
#!/bin/bash
# Search for the function containing line 218 in nix_api_flake.cc
rg -n --type=cpp "source_path.*parent\(\)" src/libflake-c/nix_api_flake.cc -B 20 -A 5Length of output: 945
Now let me check if there are similar patterns in the same file:
Based on my complete verification:
Fix critical path traversal vulnerability in nix_locked_flake_read_path.
The vulnerability is confirmed. The function accepts user-supplied path parameter and directly concatenates it with lockedFlake->lockedFlake->flake.path.parent() before calling readFile(), with no validation. This allows attackers to use sequences like "../../etc/passwd" to read arbitrary files.
The codebase provides validation utilities: canonPath() and isInDir() (available in src/libutil/include/nix/util/file-system.hh). The established pattern used elsewhere (e.g., src/libstore/unix/build/derivation-builder.cc lines 878-886) demonstrates how to apply these protections.
Recommended fix:
- Canonicalize the path using
canonPath() - Verify the result stays within the flake directory using
isInDir() - Document why
.parent()is used instead of the flake directory itself
src/libflake-c/nix_api_flake.cc
Outdated
| { | ||
| nix_clear_err(context); | ||
| try { | ||
| auto source_path = lockedFlake->lockedFlake->flake.path.parent() / path; |
There was a problem hiding this comment.
| auto source_path = lockedFlake->lockedFlake->flake.path.parent() / path; | |
| auto source_path = lockedFlake->lockedFlake->flake.path.parent() / CanonPath(path); |
Otherwise I think it may select the wrong overload for the / operator, namely:
/**
* Append a single component `c` to this path. `c` must not
* contain a slash. A slash is implicitly added between this path
* and `c`.
*/
SourcePath operator/(std::string_view c) const;
so appending a path with multiple components (foo/bar.nix) wouldn't work.
There was a problem hiding this comment.
Done, I forgot about that.
RossComputerGuy
force-pushed
the
RossComputerGuy/flake-path-capi
branch
from
23fae53 to
b04856b
Compare
Motivation
Need a way to get the path of a Flake when it is not local.
Context
Summary by CodeRabbit