feat(config): better error handling for access denied errors#1249
feat(config): better error handling for access denied errors#1249
Conversation
|
Warning Rate limit exceeded
⌛ How to resolve this issue?After the wait time has elapsed, a review can be triggered using the We recommend that you space out your commits to avoid hitting the rate limit. 🚦 How do rate limits work?CodeRabbit enforces hourly rate limits for each developer per organization. Our paid plans have higher rate limits than the trial, open-source and free plans. In all cases, we re-allow further reviews after a brief timeout. Please see our FAQ for further information. 📝 WalkthroughWalkthroughAdds a permission-diagnostics module and integrates it into DB initialization and file I/O paths to pre-check directories and wrap permission-related read/write errors with enriched diagnostic information before returning failures. Changes
Sequence DiagramsequenceDiagram
participant App as Application
participant DBInit as config/db.go
participant Perm as libs/os/perms.go
participant FS as FileSystem
participant DB as DatabaseEngine
App->>DBInit: Initialize DB (dbDir)
DBInit->>Perm: checkDBDirectory(dbDir)
Perm->>FS: stat/check access (writability)
FS-->>Perm: metadata / access result
alt permission issue
Perm->>Perm: collect file/parent/process metadata
Perm-->>DBInit: PermissionError (wrapped)
DBInit-->>App: return wrapped permission error
else directory OK
Perm-->>DBInit: success
DBInit->>DB: dbm.NewDB(..., dbDir)
DB-->>DBInit: DB handle / error
alt DB creation permission error
DBInit->>Perm: WrapPermissionError(dbPath, OperationOpenDatabase, err)
Perm-->>DBInit: wrapped PermissionError
DBInit-->>App: return wrapped error
else success
DBInit-->>App: DB ready
end
end
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes Poem
🚥 Pre-merge checks | ✅ 3✅ Passed checks (3 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ 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 |
![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
🤖 Fix all issues with AI agents
In `@libs/os/perms.go`:
- Around line 79-133: CheckFileAccess currently only stats the file and returns
nil if it exists; change it to actually verify the requested operation by
calling the OS access check (e.g., syscall.Access or
golang.org/x/sys/unix.Access) after a successful os.Stat. Map the operation
string ("read"/"write"/"exec") to the appropriate access flags (R_OK/W_OK/X_OK),
call Access(path, flags), and on error populate and return the existing
PermissionError (OriginalError, IsPermissionIssue, FileMode, FileUID, FileGID,
etc.); on success return nil. Keep the existing diagnostic population
(FileMode/FileUID/FileGID and ParentDir fields on stat failure) and only return
nil when the access check succeeds.
- Around line 170-173: The code currently ignores errors from f.Close() and
os.Remove(testFile); update the function to check both return values, e.g.
capture err := f.Close() and if err != nil return fmt.Errorf("close %s: %w",
testFile, err) (or log it), then call if err := os.Remove(testFile); err != nil
return fmt.Errorf("remove %s: %w", testFile, err); ensure you use the testFile
identifier and standard error wrapping (fmt.Errorf with %w) so failures are
propagated and leftover .tenderdash_write_test files are avoided.
- Around line 240-253: Replace the custom contains and indexOf implementations
with the standard library: add "strings" to the import block, update all call
sites that use contains(...) to use strings.Contains(...), and then remove the
contains and indexOf functions entirely (they are redundant and more complex
than strings.Contains). Ensure there are no remaining references to indexOf or
contains after the change.
- Around line 64-69: The suggested chmod -R 755 in libs/os/perms.go (the lines
building the message using e.ProcessUID, e.ProcessGID and targetDir) is unsafe
because it gives execute bits to regular files; update the suggested shell
commands to set directory and file perms separately (e.g., use find to set
directories to 755 and files to 644, or use chmod -R u=rwX,go=rX) in both the
host command and the docker exec command so only directories get execute bits
while regular files remain non-executable.
- Around line 3-9: The file perms.go uses Unix-specific types and functions
(e.g., syscall.Stat_t, syscall.Errno, syscall.EACCES, syscall.EPERM,
os.Getuid/Geteuid/Getgid/Getegid) and will fail to build on Windows; add a build
constraint by inserting //go:build !windows at the very top of perms.go to
exclude Windows builds, or alternatively create a new perms_windows.go that
defines the same exported functions/types used by callers (matching names in
perms.go) but returns errors.New("permission checking not available on Windows")
so Windows builds succeed.
🧹 Nitpick comments (5)
libs/os/perms.go (1)
233-237: Case-sensitive string fallback may miss error messages with different casing.The fallback string matching is case-sensitive. Some OS or library implementations may capitalize these messages differently (e.g.,
"Permission denied","Access Denied"). Consider usingstrings.EqualFoldor lowercasing before comparison.♻️ Proposed fix
// Check error message as fallback -errMsg := err.Error() -return contains(errMsg, "permission denied") || - contains(errMsg, "access denied") || - contains(errMsg, "operation not permitted") +errMsg := strings.ToLower(err.Error()) +return strings.Contains(errMsg, "permission denied") || + strings.Contains(errMsg, "access denied") || + strings.Contains(errMsg, "operation not permitted")types/genesis.go (1)
280-281: Double-wrapping produces a noisy error message for permission errors.
WrapPermissionErrormay return a*PermissionErrorwhoseError()is a multi-line diagnostic message. Wrapping that again withfmt.Errorf("couldn't read GenesisDoc file: %w", ...)prepends a prefix to the multi-line output, resulting in a messy message.This also differs from other call sites (e.g.,
types/node_key.goLine 120) whereWrapPermissionErroris returned directly without an additionalfmt.Errorfwrap. Consider being consistent: either always add context wrapping or rely on the operation string insideWrapPermissionError.♻️ Option A: Return the wrapped error directly (consistent with node_key.go)
jsonBlob, err := os.ReadFile(genDocFile) if err != nil { - err = tmos.WrapPermissionError(genDocFile, "read genesis file", err) - return nil, fmt.Errorf("couldn't read GenesisDoc file: %w", err) + return nil, tmos.WrapPermissionError(genDocFile, "read genesis file", err) }♻️ Option B: Only add context for non-permission errors
jsonBlob, err := os.ReadFile(genDocFile) if err != nil { - err = tmos.WrapPermissionError(genDocFile, "read genesis file", err) - return nil, fmt.Errorf("couldn't read GenesisDoc file: %w", err) + wrapped := tmos.WrapPermissionError(genDocFile, "read genesis file", err) + var permErr *tmos.PermissionError + if errors.As(wrapped, &permErr) { + return nil, wrapped + } + return nil, fmt.Errorf("couldn't read GenesisDoc file: %w", err) }config/db.go (3)
49-62:checkDBDirectorycallsCheckFileAccessredundantly.
CheckDirectoryWritable(libs/os/perms.go Line 137) already callsCheckFileAccessas its first step. The explicitCheckFileAccesscall on Line 52 is therefore redundant, doubling theos.Statsyscall.♻️ Proposed fix
// checkDBDirectory verifies the database directory exists and is accessible func checkDBDirectory(dbDir string) error { - // Check if directory exists and is accessible - if err := tmos.CheckFileAccess(dbDir, "access database directory"); err != nil { - return err - } - // Check if directory is writable if err := tmos.CheckDirectoryWritable(dbDir); err != nil { return err } return nil }
40-43: The assumed".db"suffix may not match all DB backend naming conventions.
filepath.Join(dbDir, ctx.ID+".db")is used as the path for diagnostic wrapping, but different backends use different naming (e.g., LevelDB/RocksDB use a directory, BoltDB uses a file). Since this path is only used for diagnostics inWrapPermissionError, the mismatch won't break anything, but pointing at a non-existent path may produce confusing diagnostic output. Consider usingdbDirdirectly instead, since the permission issue is almost certainly at the directory level.♻️ Proposed fix
- dbPath := filepath.Join(dbDir, ctx.ID+".db") - err = tmos.WrapPermissionError(dbPath, "open database", err) + err = tmos.WrapPermissionError(dbDir, "open database", err)
31-46: Pre-check creates and removes a test file on every DB initialization.
checkDBDirectory→CheckDirectoryWritablewrites and removes a.tenderdash_write_testfile on each call. This adds I/O overhead to every startup and could be problematic on slow or network-mounted filesystems. If the DB backend itself will promptly fail with a clear permission error, the pre-check may be unnecessary overhead. Consider whether the post-creation wrapping (Lines 39-43) alone provides sufficient diagnostics.
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 Fix all issues with AI agents
In `@libs/os/perms.go`:
- Around line 163-180: The function mapOperationToAccessFlags currently defaults
to R_OK and misses that operations like "open database" need write permission;
update mapOperationToAccessFlags to (1) normalize the input by setting op :=
strings.ToLower(operation), and (2) treat database opens as write (or
read+write) by adding "database" (or the combination "open" && "database") to
the write-check branch so it returns unix.W_OK or unix.R_OK|unix.W_OK as
appropriate; modify the condition(s) that check for "write"/"create"/"save" to
also include "database" or add a new else-if for strings.Contains(op,
"database") || (strings.Contains(op, "open") && strings.Contains(op,
"database")) to ensure opening databases maps to write access.
- Around line 80-86: The formatted help text builds shell commands using
fmt.Sprintf but mistakenly contains literal "{{}}" instead of the find
placeholder "{}", so fix the two fmt.Sprintf calls that append to msg (the ones
referencing e.ProcessUID, e.ProcessGID and targetDir) to use "{}" in the find
-exec fragments rather than "{{}}"; update both the host-chown and docker exec
strings that currently include find ... -exec chmod ... {{}} + to use find ...
-exec chmod ... {} + so the produced commands are valid, keeping all other
spacing/quoting and the variables (e.ProcessUID, e.ProcessGID, targetDir, msg)
unchanged.
🧹 Nitpick comments (1)
libs/os/perms.go (1)
44-89:Error()builds a multi-line string via repeated concatenation — considerstrings.Builder.This is a minor performance/style point. The method performs ~15 string concatenations. A
strings.Builderwould be more idiomatic and efficient for Go, though the impact is negligible since this only runs on error paths.
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Fix all issues with AI agents
In `@libs/os/perms.go`:
- Around line 121-143: The comment "File doesn't exist, check parent directory"
is misleading because this branch is executed for permission errors; update the
comment above the parentDir lookup in the isPermissionError block to reflect
that we are probing the parent directory for permission/ownership info (not
existence), e.g., "Permission denied—inspect parent directory mode/UID/GID to
diagnose access issues"; keep the logic that uses filepath.Dir(path),
os.Stat(parentDir), parentInfo.Mode(), and the syscall.Stat_t UID/GID assignment
to permErr.ParentDirMode/ParentDirUID/ParentDirGID unchanged.
🧹 Nitpick comments (2)
libs/os/perms.go (2)
26-98: Shell command in error message is vulnerable to injection ifPathcontains single quotes.The diagnostic message embeds
targetDir(derived frome.Path) inside a single-quoted shell string (lines 85, 89). If the path ever contains a', it breaks out of the quoting context, producing a malformed or potentially dangerous command for the user to copy-paste.Risk is low (paths are application-controlled), but it's straightforward to sanitize:
🛡️ Proposed fix
+ // Escape single quotes in targetDir for safe shell embedding + safeDir := strings.ReplaceAll(targetDir, "'", "'\\''") + cmd := fmt.Sprintf( - "chown -R %d:%d %s && find %s -type d -exec chmod 750 \\{\\} \\; && find %s -type f -exec chmod 640 \\{\\} \\;", - e.ProcessUID, e.ProcessGID, targetDir, targetDir, targetDir) + "chown -R %d:%d '%s' && find '%s' -type d -exec chmod 750 {} + && find '%s' -type f -exec chmod 640 {} +", + e.ProcessUID, e.ProcessGID, safeDir, safeDir, safeDir)This also:
- Quotes the directory argument to handle paths with spaces.
- Simplifies the
{}escaping (not special inside single quotes passed tobash -c/sh -c).- Uses
+terminator instead of\;for betterfindperformance (batcheschmodcalls).
54-62:FileMode == 0as a proxy for "file doesn't exist" is fragile.A file with permission mode
----------(0000) is technically valid and would also satisfye.FileMode != 0 → false, causing the diagnostics to incorrectly report "does not exist yet." Practically very rare, but a dedicatedboolfield (e.g.,FileExists) would make the intent unambiguous and avoid the edge case.Also applies to: 70-78
Issue being fixed or feature implemented
Users sometimes report invalid permission issue after upgrade.
What was done?
Better error handling: detect and suggest workaround.
How Has This Been Tested?
Manually, on local env, with docker container started manually:
Then chown'ed files, restarted the container and fixed permissions.
Breaking Changes
None
Checklist:
For repository code-owners and collaborators only
Summary by CodeRabbit
New Features
Bug Fixes