feat: Add trix encryption and format #16
Conversation
This commit introduces the `Enchantrix` library to add support for the `.trix` encrypted file format. The main changes are: - The `matrix` format has been renamed to `tim` (Terminal Isolation Matrix). - The `.tim` format is now a specialized `.trix` file. - A new `decode` command has been added to decode `.trix` and `.tim` files. - The `collect` commands now support the `trix` and `tim` formats. - A `--password` flag has been added to the `collect` commands for encryption. - A `--i-am-in-isolation` flag has been added to the `decode` command for safely decoding `.tim` files. - The decryption functionality is currently disabled due to a bug in the `Enchantrix` library. A follow-up PR will be created to re-enable it.
📝 WalkthroughSummary by CodeRabbitRelease NotesNew Features
Documentation & Examples
WalkthroughReplaces the old "matrix" workflow with two formats: "tim" (tar-based Terminal Isolation Matrix) and "trix" (encoded/encrypted variant). Adds a password flag, new tim/trix packages, a decode CLI command, updates collectors and run/compile commands, and removes the matrix package and related examples/tests. Changes
Sequence Diagram(s)sequenceDiagram
participant User
participant CLI
participant DN as DataNode
participant TIM as tim package
participant TRIX as trix package
rect rgb(210,235,255)
Note over User,CLI: Create TIM
User->>CLI: borg collect --format tim ...
CLI->>DN: build DataNode from source
DN-->>CLI: DataNode
CLI->>TIM: FromDataNode(dn) -> ToTar()
TIM-->>CLI: tar bytes
CLI->>User: write .tim file
end
rect rgb(235,210,255)
Note over User,TRIX: Create TRIX (encrypted)
User->>CLI: borg collect --format trix --password secret
CLI->>DN: build DataNode
DN-->>CLI: DataNode
CLI->>TRIX: ToTrix(dn, password)
TRIX-->>CLI: trix bytes
CLI->>User: write .trix file
end
rect rgb(255,235,210)
Note over User,TRIX: Decode (.tim/.trix)
User->>CLI: borg decode file.trix --password secret
CLI->>TRIX: FromTrix(data, password)
TRIX-->>CLI: DataNode
CLI->>TIM: ToTar/from DataNode -> extract
CLI->>User: write decoded.dat
end
Possibly related PRs
Poem
Pre-merge checks and finishing touches❌ Failed checks (1 warning)
✅ Passed checks (2 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 |
Codecov Report❌ Patch coverage is
📢 Thoughts on this report? Let us know! |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 6
🧹 Nitpick comments (9)
pkg/tim/config.go (1)
5-10: Clarify the function comment.The comment states "Returns a default runc spec" but the function actually returns a
*trix.Trixinstance. This could confuse future maintainers about the function's purpose.Apply this diff to clarify the comment:
-// DefaultSpec returns a default runc spec. +// defaultConfig returns a default Trix configuration for TIM initialization. func defaultConfig() (*trix.Trix, error) {cmd/decode_test.go (1)
12-44: Consider verifying decoded content.The test successfully validates that the decode command executes without error and produces an output file. However, it doesn't verify that the decoded content matches the original data.
Consider adding content verification:
// Verify the output file. - _, err = os.Stat(outputFile) - if err != nil { - t.Fatalf("output file not found: %v", err) + outputData, err := os.ReadFile(outputFile) + if err != nil { + t.Fatalf("failed to read output file: %v", err) + } + + // Verify the decoded tarball contains the expected file + // (Could use tar.Reader to inspect contents) + if len(outputData) == 0 { + t.Error("output file is empty") }cmd/run_test.go (1)
39-69: Make the “Ugly” test independent of the realruncbinary
TestRunCmd_Uglycurrently relies on the defaulttim.ExecCommand, which will try to execute a realruncbinary. This makes the test environment‑dependent and means the failure is more about process execution than about handling an invalid tim payload.Consider reusing the
ExecCommandmocking pattern fromTestRunCmd_Goodhere and having the helper process explicitly fail for the “invalid tim” case, so the test reliably asserts the command’s error path without depending on host tooling.cmd/collect_website.go (1)
40-44: Tidy up format handling and password UXTwo small, non‑blocking suggestions:
- The local variable
timin theformat == "tim"branch shadows the imported package name, which is slightly confusing. Renaming it (e.g.matrixortimImage) would improve readability.- Format literals (
"datanode","tim","trix") and the password semantics are now repeated across several commands. If trix is intended to always be encrypted, you may want to enforce a non‑empty password whenformat == "trix"or at least clarify in the flag help that password affects only trix output and that an empty password yields an unencrypted trix file.Extracting shared constants/helpers for formats and documenting the trix+password behaviour once would also reduce the risk of future drift between commands.
Also applies to: 59-79, 104-107
cmd/collect_github_repo.go (1)
38-42: Minor clean‑ups: avoid shadowingtimand clarify password usageA couple of small improvements you might consider:
- The
timlocal variable inif format == "tim"hides the imported package; renaming it (e.g.timImage) would avoid confusion and make stack traces a bit clearer.- As with the website command, password is only meaningful for
format == "trix". If trix is meant to always be encrypted, enforcing a non‑empty password (or at least warning) would prevent accidentally producing unencrypted trix artefacts. If unencrypted trix is supported by design, you might tweak the flag description to say “Password for trix encryption (ignored for other formats)” for clarity.Given the same literals and behaviour appear in multiple commands, centralising allowed formats and the password description in a shared helper/constant would help keep everything in sync.
Also applies to: 62-82, 105-109
cmd/collect_pwa.go (1)
59-68: Consolidate shared validation/format logic and avoidtimshadowingSome small, non‑urgent tidy‑ups:
- The format/compression validation and the tim/trix/datanode serialisation branches here are almost identical to those in the website and GitHub collectors. Extracting a helper (or at least shared constants for valid formats) would reduce duplication and keep behaviour aligned.
- The local
timvariable in theformat == "tim"branch hides the imported package; renaming (e.g.timImage) would read more clearly.- Flag descriptions still say “Output file for the DataNode”, but the output may now be tim or trix. Updating the help text to mention all supported formats would give users a more accurate mental model.
None of these affect correctness, but they would improve maintainability and CLI UX.
Also applies to: 83-103, 48-53
cmd/all.go (1)
34-38: Polish around format defaults, naming, and password behaviourA few small improvements you might want to consider:
- The
outputflag default remains"all.dat"even when--format=timor--format=trix, unlike the other commands which derive the default filename from the chosen format/compression. For consistency and user clarity, you could either adjust the default per format or document that callers are expected to override it when not using datanode.- The local
timvariable in theformat == "tim"branch shadows the imported package; renaming it (e.g.timImage) would align with the other suggestions and avoid minor confusion.- As with the other commands, password only has an effect for trix; depending on the intended security guarantees, you may want to enforce a non‑empty password when
format == "trix"or clarify in the flag help that password is only used for trix and that an empty password yields an unencrypted trix archive.These are UX/maintainability improvements rather than correctness issues, but addressing them would make the CLI more predictable across commands.
Also applies to: 106-126, 143-147
cmd/decode.go (1)
49-49: Consider more restrictive file permissions for decoded data.The decoded output uses
0644permissions, making it readable by all users. Since the data may have been encrypted (implying sensitivity), consider using0600to restrict access to the file owner only.Apply this diff:
- return os.WriteFile(outputFile, tarball, 0644) + return os.WriteFile(outputFile, tarball, 0600)pkg/tim/run_test.go (1)
10-32: Consider verifying the mocked command invocation.The test currently only asserts that
Runreturns no error, but doesn't verify that the correct command (runc run -b <tempDir> borg-container) was invoked with the expected arguments. Whilst the current test provides basic coverage, capturing and asserting the command and arguments would strengthen confidence in the implementation.Example enhancement:
+ var capturedCmd string + var capturedArgs []string ExecCommand = func(command string, args ...string) *exec.Cmd { + capturedCmd = command + capturedArgs = args cs := []string{"-test.run=TestHelperProcess", "--", command} cs = append(cs, args...) cmd := exec.Command(os.Args[0], cs...) cmd.Env = []string{"GO_WANT_HELPER_PROCESS=1"} return cmd } t.Cleanup(func() { ExecCommand = origExecCommand }) // Run the run command. err := Run(timPath) if err != nil { t.Fatalf("run command failed: %v", err) } + + // Verify the correct command was invoked + if capturedCmd != "runc" { + t.Errorf("expected command 'runc', got '%s'", capturedCmd) + } + if len(capturedArgs) < 2 || capturedArgs[0] != "run" || capturedArgs[1] != "-b" { + t.Errorf("unexpected arguments: %v", capturedArgs) + }
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (2)
go.sumis excluded by!**/*.sumgo.workis excluded by!**/*.work
📒 Files selected for processing (33)
cmd/all.go(4 hunks)cmd/collect_github_repo.go(4 hunks)cmd/collect_pwa.go(4 hunks)cmd/collect_website.go(4 hunks)cmd/compile.go(3 hunks)cmd/compile_test.go(1 hunks)cmd/decode.go(1 hunks)cmd/decode_test.go(1 hunks)cmd/run.go(1 hunks)cmd/run_test.go(3 hunks)docs/index.md(1 hunks)examples/create_matrix.sh(0 hunks)examples/create_matrix_programmatically/main.go(0 hunks)examples/create_tim.sh(1 hunks)examples/create_tim_programmatically/main.go(1 hunks)examples/run_matrix_programmatically/main.go(0 hunks)examples/run_tim_programmatically/main.go(1 hunks)examples/serve_matrix.sh(0 hunks)examples/serve_tim.sh(1 hunks)go.mod(1 hunks)pkg/datanode/datanode.go(2 hunks)pkg/matrix/config.go(0 hunks)pkg/matrix/matrix_more_test.go(0 hunks)pkg/matrix/matrix_test.go(0 hunks)pkg/matrix/run.go(0 hunks)pkg/matrix/run_test.go(0 hunks)pkg/tim/config.go(1 hunks)pkg/tim/run.go(1 hunks)pkg/tim/run_test.go(1 hunks)pkg/tim/tim.go(3 hunks)pkg/tim/tim_more_test.go(1 hunks)pkg/tim/tim_test.go(1 hunks)pkg/trix/trix.go(1 hunks)
💤 Files with no reviewable changes (9)
- pkg/matrix/matrix_test.go
- pkg/matrix/matrix_more_test.go
- examples/run_matrix_programmatically/main.go
- pkg/matrix/run.go
- examples/create_matrix.sh
- pkg/matrix/config.go
- pkg/matrix/run_test.go
- examples/serve_matrix.sh
- examples/create_matrix_programmatically/main.go
🧰 Additional context used
🧬 Code graph analysis (17)
pkg/datanode/datanode.go (1)
pkg/tim/tim.go (1)
New(25-42)
cmd/compile.go (2)
pkg/datanode/datanode.go (1)
New(27-29)pkg/tim/tim.go (1)
New(25-42)
pkg/tim/tim_test.go (2)
pkg/datanode/datanode.go (1)
New(27-29)pkg/tim/tim.go (4)
New(25-42)FromDataNode(45-55)ErrDataNodeRequired(14-14)ErrConfigIsNil(15-15)
cmd/all.go (2)
pkg/tim/tim.go (1)
FromDataNode(45-55)pkg/trix/trix.go (1)
ToTrix(10-33)
pkg/tim/tim_more_test.go (2)
pkg/tim/tim.go (3)
FromDataNode(45-55)New(25-42)TerminalIsolationMatrix(19-22)pkg/datanode/datanode.go (2)
DataNode(22-24)New(27-29)
cmd/decode_test.go (3)
pkg/datanode/datanode.go (1)
New(27-29)pkg/trix/trix.go (1)
ToTrix(10-33)cmd/decode.go (1)
NewDecodeCmd(14-56)
pkg/trix/trix.go (1)
pkg/datanode/datanode.go (2)
DataNode(22-24)FromTar(32-55)
cmd/collect_github_repo.go (2)
pkg/tim/tim.go (1)
FromDataNode(45-55)pkg/trix/trix.go (1)
ToTrix(10-33)
pkg/tim/run_test.go (1)
pkg/tim/run.go (2)
ExecCommand(12-12)Run(15-60)
examples/run_tim_programmatically/main.go (1)
pkg/tim/run.go (1)
Run(15-60)
examples/create_tim_programmatically/main.go (2)
pkg/datanode/datanode.go (1)
New(27-29)pkg/tim/tim.go (2)
New(25-42)FromDataNode(45-55)
cmd/decode.go (2)
pkg/trix/trix.go (1)
FromTrix(36-53)cmd/root.go (1)
RootCmd(23-23)
cmd/run_test.go (2)
pkg/tim/run.go (2)
ExecCommand(12-12)Run(15-60)cmd/run.go (1)
GetRunCmd(21-23)
cmd/run.go (1)
pkg/tim/run.go (1)
Run(15-60)
cmd/compile_test.go (1)
cmd/compile.go (1)
NewCompileCmd(17-66)
cmd/collect_pwa.go (3)
pkg/pwa/pwa.go (1)
PWAClient(18-21)pkg/tim/tim.go (1)
FromDataNode(45-55)pkg/trix/trix.go (1)
ToTrix(10-33)
cmd/collect_website.go (2)
pkg/tim/tim.go (1)
FromDataNode(45-55)pkg/trix/trix.go (1)
ToTrix(10-33)
🪛 LanguageTool
docs/index.md
[style] ~38-~38: Three successive sentences begin with the same word. Consider rewording the sentence or use a thesaurus to find a synonym.
Context: ...lease.dat ``` #### borg collect pwa This command collects a Progressive Web App ...
(ENGLISH_WORD_REPEAT_BEGINNING_RULE)
[style] ~46-~46: Three successive sentences begin with the same word. Consider rewording the sentence or use a thesaurus to find a synonym.
Context: ...h.dat ``` #### borg collect website This command collects a single website and s...
(ENGLISH_WORD_REPEAT_BEGINNING_RULE)
[style] ~133-~133: Using two adverbs in a row like this may make your sentence unnecessarily wordy. For clarity and conciseness, see if you can omit one of them.
Context: ...is a safety measure to prevent you from accidentally executing potentially malicious code on your host system. ``...
(LY_LY)
🔇 Additional comments (27)
pkg/datanode/datanode.go (1)
6-19: LGTM!The addition of password-related error variables follows Go conventions. The errors are properly exported and use clear, descriptive messages suitable for the new encryption functionality.
examples/serve_tim.sh (1)
1-12: LGTM!The example script clearly demonstrates the tim workflow: collecting a repository as a .tim file and then serving it. The commands align with the new format support introduced in this PR.
examples/create_tim.sh (1)
1-8: LGTM!The example script provides a clear, minimal demonstration of creating a .tim file using the new format flag.
docs/index.md (1)
1-137: LGTM!The documentation clearly explains the new tim and trix formats, including encryption and decoding workflows. The examples are helpful and align with the CLI changes in this PR.
Note: Static analysis flagged minor style issues (repeated sentence beginnings at lines 38, 46 and adverb usage at line 133), but these don't impact clarity.
pkg/tim/tim_more_test.go (3)
10-26: LGTM!The tests properly validate the happy path for
FromDataNode, checking error handling, nil results, and correct RootFS assignment.
28-34: LGTM!The test correctly validates that
ToTarexecutes without error on a properly constructed TIM.
36-54: LGTM!The helper functions are well-structured, properly use
t.Helper(), and provide reusable test fixtures.pkg/tim/run.go (2)
11-13: LGTM!The exported
ExecCommandvariable follows a common Go testing pattern, allowing tests to mock command execution.
55-59: LGTM!The runc command execution properly wires stdout and stderr to the host and uses the mockable
ExecCommandvariable for testability.go.mod (2)
6-6: Clarify Enchantrix dependency version and bug documentation.The dependency uses a pseudo-version pointing to commit deff3a80c600 (a test-adding merge from 2025-11-13), not a production release. Whilst the PR description mentions a decryption bug in Enchantrix, no corresponding issue or fix is tracked in the Enchantrix repository—no releases exist and recent commits focus on test improvements, not bug resolution.
Before proceeding, please:
- Confirm whether the decryption bug is tracked (provide issue/PR link)
- Clarify whether a stable Enchantrix release is available or if this development version is intentional
- Document any security implications of using a pseudo-version from an untagged commit
14-43: No security vulnerabilities found in the specified dependency versions.golang.org/x/net v0.47.0 is at or above the fixed version for CVE-2024-45338, and golang.org/x/oauth2 v0.33.0 is above the fixed version (0.27.0) for CVE-2025-22868. No known vulnerabilities exist for golang.org/x/mod v0.30.0 or the other indirect dependencies listed. The dependency updates maintain security compliance.
cmd/run.go (1)
4-16: LGTM! Clean refactoring to tim package.The import, usage string, and function call have been correctly updated to reflect the migration from matrix to tim format. The change is consistent with the broader refactoring described in the PR objectives.
cmd/compile.go (1)
8-64: LGTM! Consistent migration to tim package.The changes correctly update the import path, API usage, and default output extension from matrix to tim format. The refactoring maintains the existing command behaviour whilst adopting the new format.
pkg/tim/tim.go (2)
144-147: Acknowledged: ToTrix method commented out.The commented-out
ToTrixmethod aligns with the PR description noting that decryption functionality is currently disabled due to a bug in the Enchantrix library. This will be re-enabled in a follow-up PR.
28-28: defaultConfig() function exists and is properly defined.The function has been successfully verified in pkg/tim/config.go at line 6, returning (*trix.Trix, error). The function call on line 28 of pkg/tim/tim.go is valid.
examples/run_tim_programmatically/main.go (1)
1-16: LGTM! Clear example demonstrating tim execution.The example correctly demonstrates how to programmatically execute a tim file using the Borg package. Error handling with
log.Fatalfis appropriate for example code.pkg/tim/tim_test.go (1)
11-73: LGTM! Comprehensive test coverage.The tests provide good coverage of the tim package's core functionality:
TestNewvalidates initialisation and structureTestFromDataNodetests both success and nil input error handlingTestToTarverifies serialisation and nil config error handlingThe use of subtests and explicit error checking follows best practices.
examples/create_tim_programmatically/main.go (1)
11-35: LGTM! Clear example demonstrating tim creation.The example effectively demonstrates the complete workflow:
- Creating a DataNode and adding data
- Constructing a TerminalIsolationMatrix from the DataNode
- Serialising to tarball format
- Writing to a file
Error handling with
log.Fatalfis appropriate for example code, and the output provides clear feedback.cmd/run_test.go (1)
13-37: Good coverage of happy path and realistic tim fixtureThe
TestRunCmd_Goodflow together withcreateDummyTimgives a solid end‑to‑end check of the Cobra command wiring and thetim.Runpath (including theExecCommandhook), while keeping it hermetic via the helper process. The tar structure increateDummyTimis minimal but sufficient and reads cleanly.Also applies to: 71-112
cmd/collect_website.go (1)
28-59: Website collection flow for tim/trix looks soundThe addition of tim/trix here is wired cleanly: early format validation,
website.DownloadAndPackageWebsitefeeding into eithertim.FromDataNode().ToTar,trix.ToTrix(password), ordn.ToTar(), followed by a single compression/write path. Error wrapping is consistent and makes it clear which stage failed, and the default output naming (website.+format+compression) is intuitive.Also applies to: 81-99
cmd/collect_github_repo.go (1)
26-43: GitHub repo collector’s tim/trix branches are correctly integratedThe new format handling is coherent: early validation of
format/compression, consistent use oftim.FromDataNode().ToTarvstrix.ToTrix(password)vsdn.ToTar(), and clear error messages for each stage. Default filenames and compression behaviour are predictable, and the flow matches the other collectors, which should make it easier to reason about end‑to‑end behaviour.Also applies to: 62-83, 89-103, 105-109
cmd/collect_pwa.go (1)
21-53: Nice separation of CLI wiring from CollectPWA logicPulling the core logic into
CollectPWAwith an explicitPWAClientand all CLI parameters (includingpassword) makes this code much easier to test in isolation and keeps theRunEhandler slim. The flow of manifest discovery → PWA packaging → format‑specific serialisation → compression and write is clear, and the error messages give good context at each stage.Also applies to: 56-58, 70-82, 105-122
cmd/all.go (1)
23-39: All‑in‑one collector’s tim/trix integration is coherentThe new tim/trix support is slotted into the existing “collect all repos then serialise once” flow cleanly: format validation happens up front, repositories are still merged via
allDataNodes, and the final serialisation path branches totim.FromDataNode().ToTar,trix.ToTrix(password), orallDataNodes.ToTar()before compression and write. Error messages clearly distinguish tim/trix creation from generic DataNode serialisation, which should help with diagnostics.Also applies to: 60-62, 106-126, 143-147
cmd/decode.go (1)
35-37: Good safety mechanism for Terminal Isolation Matrix.The explicit flag requirement for decoding
.timfiles provides a clear safety barrier, preventing accidental execution of potentially dangerous isolated environments.pkg/trix/trix.go (1)
10-33: LGTM: Encryption path is correctly implemented.The
ToTrixfunction properly converts aDataNodeto a tarball, conditionally encrypts it when a password is provided, and encodes it to the TRIX format.pkg/tim/run_test.go (2)
34-74: Well-structured test helper.The
createDummyTimhelper correctly usest.Helper(), creates a minimal valid tim archive with the expected structure (config.jsonandrootfs/directory), and includes appropriate error handling.
76-84: LGTM: Standard test helper pattern.The
TestHelperProcessfunction correctly implements the standard Go pattern for mockingexec.Commandin tests.
| outputFile, _ := cmd.Flags().GetString("output") | ||
| password, _ := cmd.Flags().GetString("password") | ||
| inIsolation, _ := cmd.Flags().GetBool("i-am-in-isolation") |
There was a problem hiding this comment.
Handle errors from flag getters.
While errors are unlikely for flags defined on this command, ignoring error returns violates best practices and could mask configuration issues in future refactoring.
Apply this diff to handle the errors:
- outputFile, _ := cmd.Flags().GetString("output")
- password, _ := cmd.Flags().GetString("password")
- inIsolation, _ := cmd.Flags().GetBool("i-am-in-isolation")
+ outputFile, err := cmd.Flags().GetString("output")
+ if err != nil {
+ return err
+ }
+ password, err := cmd.Flags().GetString("password")
+ if err != nil {
+ return err
+ }
+ inIsolation, err := cmd.Flags().GetBool("i-am-in-isolation")
+ if err != nil {
+ return err
+ }📝 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.
| outputFile, _ := cmd.Flags().GetString("output") | |
| password, _ := cmd.Flags().GetString("password") | |
| inIsolation, _ := cmd.Flags().GetBool("i-am-in-isolation") | |
| outputFile, err := cmd.Flags().GetString("output") | |
| if err != nil { | |
| return err | |
| } | |
| password, err := cmd.Flags().GetString("password") | |
| if err != nil { | |
| return err | |
| } | |
| inIsolation, err := cmd.Flags().GetBool("i-am-in-isolation") | |
| if err != nil { | |
| return err | |
| } |
🤖 Prompt for AI Agents
In cmd/decode.go around lines 21 to 23, the returned errors from
cmd.Flags().GetString/GetBool are ignored; update the code to capture each error
and handle it (e.g., if err != nil { return fmt.Errorf("failed to read %s flag:
%w", "<flag-name>", err) } or log/fatal according to this command's error
handling pattern) for "output", "password", and "i-am-in-isolation" so flag
retrieval failures are surfaced rather than silently ignored.
This commit introduces the `Enchantrix` library to add support for the `.trix` encrypted file format. The main changes are: - The `matrix` format has been renamed to `tim` (Terminal Isolation Matrix). - The `.tim` format is now a specialized `.trix` file. - A new `decode` command has been added to decode `.trix` and `.tim` files. - The `collect` commands now support the `trix` and `tim` formats. - A `--password` flag has been added to the `collect` commands for encryption. - A `--i-am-in-isolation` flag has been added to the `decode` command for safely decoding `.tim` files. - The decryption functionality is currently disabled due to a bug in the `Enchantrix` library. A follow-up PR will be created to re-enable it. - Path traversal vulnerability in `pkg/tim/run.go` has been fixed. - File descriptor leak in `pkg/tim/run.go` has been fixed. - Improved error handling in `pkg/trix/trix.go`.
There was a problem hiding this comment.
Actionable comments posted: 1
♻️ Duplicate comments (1)
pkg/tim/run.go (1)
34-37: Critical issue not fully addressed: tar read errors are silently ignored.The previous review requested distinguishing between
io.EOFand actual errors, but the current code still breaks on any error without checking. Real tar read errors (corrupted archives, I/O failures) will be silently ignored instead of being reported.Apply this diff to properly handle tar errors:
for { hdr, err := tr.Next() + if err == io.EOF { + break + } if err != nil { - break + return fmt.Errorf("failed to read tar: %w", err) }
🧹 Nitpick comments (2)
pkg/trix/trix.go (1)
10-34:ToTrixflow and error handling look sound; only minor future-proofing to considerThe conversion → optional encryption → TRIX encoding pipeline is clear and errors are correctly propagated. This is a good, minimal abstraction for callers.
If you expect to support multiple encryption modes or need to detect “encrypted vs plain” payloads at decode time later, consider (optionally) recording lightweight metadata in
t.Header(for example anencrypted: trueflag and/or a version marker) so future decode logic does not have to infer state purely from whether a password was supplied. This is non‑blocking but may save you a format migration later.pkg/tim/run.go (1)
39-43: Usefilepath.IsLocal()for cleaner path traversal protection.Your Go version (1.25.0) supports
filepath.IsLocal(), which is the idiomatic approach for this validation. The current implementation works correctly but is more complex than necessary. Replace the string-prefix checks with a simple call tofilepath.IsLocal():+ // Prevent path traversal attacks + if !filepath.IsLocal(hdr.Name) { + return fmt.Errorf("invalid file path: %s", hdr.Name) + } target := filepath.Join(tempDir, hdr.Name) - target = filepath.Clean(target) - if !strings.HasPrefix(target, filepath.Clean(tempDir)+string(os.PathSeparator)) && target != filepath.Clean(tempDir) { - return fmt.Errorf("invalid file path: %s", hdr.Name) - }This eliminates the manual string manipulation whilst maintaining equivalent security guarantees.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
pkg/tim/run.go(1 hunks)pkg/trix/trix.go(1 hunks)
🧰 Additional context used
🧬 Code graph analysis (1)
pkg/trix/trix.go (1)
pkg/datanode/datanode.go (2)
DataNode(22-24)FromTar(32-55)
🔇 Additional comments (3)
pkg/tim/run.go (3)
3-15: LGTM: Imports and test setup are correct.The imports now include
stringsandioas needed. TheExecCommandvariable pattern is excellent for enabling test mocking.
50-64: Excellent fix: resource leak resolved.The file handle is now closed immediately after writing rather than being deferred in the loop, completely addressing the previous resource leak issue. Both
io.CopyandCloseerrors are properly handled.
68-72: LGTM: runc execution is properly configured.The container execution correctly uses the unpacked temporary directory as the bundle and properly wires stdout/stderr for visibility.
| // FromTrix converts a Trix byte slice back to a DataNode. | ||
| func FromTrix(data []byte, password string) (*datanode.DataNode, error) { | ||
| // Decode the Trix byte slice. | ||
| t, err := trix.Decode(data, "TRIX", nil) | ||
| if err != nil { | ||
| return nil, err | ||
| } | ||
|
|
||
| // Decrypt the payload if a password is provided. | ||
| if password != "" { | ||
| return nil, fmt.Errorf("decryption disabled: cannot accept encrypted payloads") | ||
| } | ||
|
|
||
| // Convert the tarball back to a DataNode. | ||
| return datanode.FromTar(t.Payload) | ||
| } |
There was a problem hiding this comment.
Comment in FromTrix no longer matches behaviour; update to avoid confusion
The new password != "" guard correctly prevents the old bug where encrypted payloads were parsed as tar, and aligns with the “decryption disabled” state in the PR description. However, the inline comment still says:
// Decrypt the payload if a password is provided.
which is now misleading, as you immediately return an error instead of decrypting. Updating the comment to reflect the current behaviour will avoid confusion for future maintainers and for the follow‑up PR that re‑enables decryption.
For example:
- // Decrypt the payload if a password is provided.
+ // Decryption is currently disabled; reject calls that provide a password.
if password != "" {
- return nil, fmt.Errorf("decryption disabled: cannot accept encrypted payloads")
+ return nil, fmt.Errorf("decryption disabled: cannot accept encrypted payloads")
}📝 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.
| // FromTrix converts a Trix byte slice back to a DataNode. | |
| func FromTrix(data []byte, password string) (*datanode.DataNode, error) { | |
| // Decode the Trix byte slice. | |
| t, err := trix.Decode(data, "TRIX", nil) | |
| if err != nil { | |
| return nil, err | |
| } | |
| // Decrypt the payload if a password is provided. | |
| if password != "" { | |
| return nil, fmt.Errorf("decryption disabled: cannot accept encrypted payloads") | |
| } | |
| // Convert the tarball back to a DataNode. | |
| return datanode.FromTar(t.Payload) | |
| } | |
| // FromTrix converts a Trix byte slice back to a DataNode. | |
| func FromTrix(data []byte, password string) (*datanode.DataNode, error) { | |
| // Decode the Trix byte slice. | |
| t, err := trix.Decode(data, "TRIX", nil) | |
| if err != nil { | |
| return nil, err | |
| } | |
| // Decryption is currently disabled; reject calls that provide a password. | |
| if password != "" { | |
| return nil, fmt.Errorf("decryption disabled: cannot accept encrypted payloads") | |
| } | |
| // Convert the tarball back to a DataNode. | |
| return datanode.FromTar(t.Payload) | |
| } |
🤖 Prompt for AI Agents
In pkg/trix/trix.go around lines 36 to 51 the inline comment "// Decrypt the
payload if a password is provided." is outdated and misleading because the
function immediately returns an error when password != "" (decryption is
disabled); update that comment to accurately state that encrypted payloads are
rejected and decryption is currently disabled (e.g., "// Reject encrypted
payloads: decryption is disabled and an error is returned if a password is
provided.") so future maintainers understand the current behavior.
This commit introduces the
Enchantrixlibrary to add support for the.trixencrypted file format.The main changes are:
matrixformat has been renamed totim(Terminal Isolation Matrix)..timformat is now a specialized.trixfile.decodecommand has been added to decode.trixand.timfiles.collectcommands now support thetrixandtimformats.--passwordflag has been added to thecollectcommands for encryption.--i-am-in-isolationflag has been added to thedecodecommand for safely decoding.timfiles.Enchantrixlibrary. A follow-up PR will be created to re-enable it.