Repository navigation
Sprint review: a read error no longer wipes an ini file; blank or relative path variables count as unset - #53
Merged
Merged
Conversation
…ative XDG paths count as unset From an independent adversarial review of #34 #36 #39 #42 #44 #46 #47 #49. - ConfigStore read any OSError opening its file as 'missing', so a transient error (too many open files, permissions) made it read as empty and the next write replaced the whole file. Only a missing file is skipped now; ConfigReader, which reports unusable sources per on_missing and never writes back, still tolerates them. - A blank or relative XDG_* (or CONFIG2PY_*_DIR) value rooted config folders, secrets included, at the cwd. As the XDG spec says, such a value now counts as unset (#38). - A value that can't be encoded raised a KeyError quoting the offending character of the (possibly secret) value; it now names only the error type and position. - check_folder_is_private skips the folder check once a folder was warned about (it could call pwd.getpwall on every store construction), and a bad on_unsafe value is reported under its own name. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
…empty; ini persist doesn't quote the character From the independent review of the previous commit. - A non-empty XDG_* or CONFIG2PY_*_DIR value that is ignored (relative) now warns, so secrets never land somewhere the user didn't choose in silence. - ConfigStore again reads a folder, a path through a file, or a content string too long to be a name as empty (none is a file a write could lose); only real read errors (permissions, too many open files) raise. - ConfigStore.persist raised a UnicodeEncodeError quoting the character it can't encode; it still raises that type, with the character replaced. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
Windows reports a folder as PermissionError and an over-long name as EINVAL, so matching error types was not portable. The rule is now the risk itself: only an existing regular file that a store failed to read raises, since its next write would replace it. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Sprint review (independent of the sprint's authors) of #34, #36, #39, #42, #44, #46, #47 and #49. An Opus adversarial reviewer tried each attack on the secure-write code and none got through: permissions under
umask 0, files planted by another user, symlinks, hard links, thread safety of the lazy instances. It did find gaps around them:ConfigStoretreated anyOSErroras "file missing". A transient error (too many open files, permissions) made it read as empty, and the next write replaced the whole file. The review reproduced anapi_keysection lost this way. Now only a missing file, or a source that is no file at all, reads as empty, and real read errors raise.ConfigReaderreports unusable sources peron_missingand never writes back, so it still tolerates them.XDG_*andCONFIG2PY_*_DIR. Such a value now counts as unset, as the XDG spec says (Empty XDG_* env var resolves app folders relative to the cwd #38), and a non-empty one that is ignored warns.SecureTextFilesandConfigStore.persistnow report the position only, and the exception types are unchanged.check_folder_is_privateno longer re-checks a folder it already warned about; the check can callpwd.getpwall()on every store construction. A badon_unsafevalue is now reported under its own name.Filed: #52, secret writes are not atomic, so a crash mid-write leaves an empty value file. This predates the sprint.
Dependents: no dependent constructs
ConfigStoreover an unreadable file, matches on the error texts, or sets a relativeCONFIG2PY_*(checked with rg across the fleet). The only override in the fleet, in oa's tests, uses an absolute path.Tests: 250 before, 254 after, run as CI does (
--doctest-modules, sandboxed HOME/XDG). An independent review of the fix found no blocker; its two should-fixes and two nits are in the second commit.🤖 Generated with Claude Code