Keep four commands from building a path that climbs out of its directory - #9425
Open
VykosMolt wants to merge 1 commit into
Open
Keep four commands from building a path that climbs out of its directory#9425VykosMolt wants to merge 1 commit into
VykosMolt wants to merge 1 commit into
Conversation
omarchy-hook, omarchy-hyprland-toggle, omarchy-toggle and omarchy-state each
join an argument straight into a path they then execute, delete or write, with
nothing stopping the argument from leaving the directory it was meant for.
Against a sandboxed HOME on current quattro:
omarchy-hook ../../../../outside-hook
runs that script, four directories above the hooks directory
omarchy-hyprland-toggle ../../../../../../outside off
deletes outside.lua, six above the toggles directory
omarchy-toggle ../../../../../outside-toggle on
creates a file five above the state directory
omarchy-state set ../../../../outside-state
creates a file four above the state directory
The hook is the worst of the four, because it is execution rather than a write.
omarchy-done already refuses `.`, `..` and a name holding a slash, so the rule
was already in the tree, in one command, inline. It cannot be that rule exactly:
omarchy-crash-mute keeps a flag per program under `crash-ignore/<program>`, and
omarchy-crash-watch reads it back the same way, so omarchy-toggle has to keep
accepting a slash. What climbs is a `.` or `..` component, not a separator.
omarchy-path-guard holds that rule once and the four commands source it. A name
is refused when it is empty, when it is absolute, or when any component of it is
`.` or `..`, which leaves `crash-ignore/chromium` working and rejects
`crash-ignore/../../escape`. Ordinary filenames that merely start with dots
(`...`, `..leading`) stay legal, and so do the glob characters omarchy-state's
clear pattern is given.
Sourcing is `source omarchy-path-guard || exit 1` rather than a bare source,
because two of these commands do not set -e and a guard that can go missing
without stopping the command is not a guard.
The new tests are the four escapes above, each checked by whether the file
appears, disappears or runs, plus the crash-ignore namespace and the ordinary
names that have to keep working. The first fails on unmodified quattro.
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.
omarchy-hook,omarchy-hyprland-toggle,omarchy-toggleandomarchy-stateeach join an argument straight into a path they then execute, delete or write, and nothing stops the argument from leaving the directory it was meant for.omarchy-donealready refuses.,..and a name holding a slash, so the rule was in the tree already, in one command, written inline. It cannot be that rule exactly here:omarchy-crash-mutekeeps a flag per program undercrash-ignore/<program>andomarchy-crash-watchreads it back the same way, soomarchy-togglehas to keep accepting a slash. What climbs is a.or..component, not a separator.omarchy-path-guardholds that rule once and the four commands source it. A name is refused when it is empty, when it is absolute, or when any component of it is.or...crash-ignore/chromiumkeeps working,crash-ignore/../../escapedoes not. Ordinary filenames that merely begin with dots (...,..leading) stay legal, and so do the glob charactersomarchy-state clearis given.The source line is
source omarchy-path-guard || exit 1rather than a baresource, because two of these commands do notset -e, and a guard that can go missing without stopping the command is not a guard.Testing
Each escape below was run against unmodified quattro with
HOMEpointed at a scratch directory, and each one worked:The hook is the worst of the four, since it is execution rather than a write.
test/cligains eight assertions: the four escapes, checked by whether the file appears, disappears or runs; thatcrash-ignore/chromiumstill gets its flag; thatcrash-ignore/../../escapedoes not; thatweird...nameand..leadingare still accepted; and thatomarchy-state clear 'globb*'still matches a glob. The first fails on unmodified quattro../test/clipasses in full, including the metadata check overbin/../test/shell— 217 of 221 files pass; the four failures (config-test,runtime-smoke-test,snapper-test,unowned-system-paths-test) fail identically on unmodified quattro on this machine.crash-capture-test.sh, which exercises thecrash-ignore/namespace throughomarchy-toggle, passes with the guard in place.This comes out of https://github.com/VykosMolt/omarchy-desktop, where I have been running the Quattro shell as a plain Arch session and fixing what turned up.