Skip to content

Commit f555f6c

Browse files
claudeclaude
committed
Owen facade: address CodeRabbit round on PR #246
- ci.yml: use `python` instead of `python3` in the CLI smoke job's SARIF driver-name assertions -- actions/setup-python does not expose a python3 alias on windows-latest, so these would have broken the Windows leg. - ci.yml: the inaccessible-subtree test is now Linux-only (chmod 000 does not reliably lock out the job's own account on Windows) and actually proves the lockout worked -- asserts exit 1 and that the output names Real.cs but never Secret.cs, instead of a bare "OWN001 somewhere" check that would pass just as well if the locked file got analyzed too. - CoreVendor.cs: close the narrow TOCTOU window between the Directory.Exists(finalOwnlang) race check and Directory.Move -- a concurrent `owen` process publishing the same fingerprint in between now resolves the same way the earlier check already did (their content is provably identical), instead of surfacing Move's IOException. - CoreVendor.cs: Fingerprint hashes the MemoryStream directly (rewound) instead of copying it via ToArray() first. - owen-public-facade.md: language tag on the --help excerpt's code fence. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01SSXTDuh1ZHdQc4QqmYwshw
1 parent 9e2a758 commit f555f6c

3 files changed

Lines changed: 36 additions & 14 deletions

File tree

‎.github/workflows/ci.yml‎

Lines changed: 19 additions & 10 deletions
Original file line numberDiff line numberDiff line change
@@ -2076,7 +2076,7 @@ jobs:
20762076
- name: owen check --format sarif -- Owen-branded SARIF driver name
20772077
run: |
20782078
out=$(owen check "$RUNNER_TEMP/owen-sample" --format sarif)
2079-
echo "$out" | python3 -c "
2079+
echo "$out" | python -c "
20802080
import json, sys
20812081
d = json.load(sys.stdin)
20822082
n = d['runs'][0]['tool']['driver']['name']
@@ -2137,7 +2137,7 @@ jobs:
21372137
rm -rf "$HOME/.owen" "$HOME/.ownsharp"
21382138
mkdir -p "$HOME/.ownsharp/core/0.1.0/ownlang"
21392139
cp ownlang/*.py "$HOME/.ownsharp/core/0.1.0/ownlang/"
2140-
driver=$(owen check "$RUNNER_TEMP/owen-sample" --format sarif | python3 -c "import json,sys; print(json.load(sys.stdin)['runs'][0]['tool']['driver']['name'])")
2140+
driver=$(owen check "$RUNNER_TEMP/owen-sample" --format sarif | python -c "import json,sys; print(json.load(sys.stdin)['runs'][0]['tool']['driver']['name'])")
21412141
[ "$driver" = "Owen" ] || { echo "FAIL: driver name '$driver' via the legacy cache"; exit 1; }
21422142
if [ -d "$HOME/.owen" ]; then
21432143
echo "FAIL: ~/.owen was created despite a legitimate matching legacy cache (fallback not reused in place)"; exit 1
@@ -2152,7 +2152,7 @@ jobs:
21522152
mkdir -p "$HOME/.ownsharp/core/0.1.0/ownlang"
21532153
cp ownlang/*.py "$HOME/.ownsharp/core/0.1.0/ownlang/"
21542154
echo "# stale leftover module" > "$HOME/.ownsharp/core/0.1.0/ownlang/removed_module.py"
2155-
driver=$(owen check "$RUNNER_TEMP/owen-sample" --format sarif | python3 -c "import json,sys; print(json.load(sys.stdin)['runs'][0]['tool']['driver']['name'])")
2155+
driver=$(owen check "$RUNNER_TEMP/owen-sample" --format sarif | python -c "import json,sys; print(json.load(sys.stdin)['runs'][0]['tool']['driver']['name'])")
21562156
[ "$driver" = "Owen" ] || { echo "FAIL: driver name '$driver'"; exit 1; }
21572157
[ -d "$HOME/.owen" ] || { echo "FAIL: expected a fresh ~/.owen unpack (extra-file legacy cache should have been rejected)"; exit 1; }
21582158
if find "$HOME/.owen" -name "removed_module.py" | grep -q .; then
@@ -2167,7 +2167,7 @@ jobs:
21672167
mkdir -p "$HOME/.ownsharp/core/0.1.0/ownlang"
21682168
cp ownlang/*.py "$HOME/.ownsharp/core/0.1.0/ownlang/"
21692169
echo "# tampered" >> "$HOME/.ownsharp/core/0.1.0/ownlang/ownir.py"
2170-
driver=$(owen check "$RUNNER_TEMP/owen-sample" --format sarif | python3 -c "import json,sys; print(json.load(sys.stdin)['runs'][0]['tool']['driver']['name'])")
2170+
driver=$(owen check "$RUNNER_TEMP/owen-sample" --format sarif | python -c "import json,sys; print(json.load(sys.stdin)['runs'][0]['tool']['driver']['name'])")
21712171
[ "$driver" = "Owen" ] || { echo "FAIL: driver name '$driver'"; exit 1; }
21722172
[ -d "$HOME/.owen" ] || { echo "FAIL: expected a fresh ~/.owen unpack (modified-content legacy cache should have been rejected)"; exit 1; }
21732173
echo "OK: modified-content legacy cache correctly rejected"
@@ -2246,9 +2246,14 @@ jobs:
22462246
[ "$rc" -eq 1 ] || { echo "FAIL: expected exit 1 (real source found despite bin/ noise), got $rc"; exit 1; }
22472247
echo "$out" | grep -q "OWN001" || { echo "FAIL: expected OWN001"; exit 1; }
22482248
- name: Inaccessible subtree plus one readable source -- tolerated, real source still found
2249-
# On these hosted runners the job does not run as an all-powerful admin/
2250-
# root account, so chmod/icacls genuinely restricts access (unlike a
2251-
# root-based local sandbox, where this same scenario cannot be exercised).
2249+
# On these hosted Linux runners the job does not run as an all-powerful
2250+
# admin/root account, so chmod genuinely restricts access (unlike a
2251+
# root-based local sandbox, where this same scenario cannot be exercised,
2252+
# and unlike Windows runners, where chmod 000 does not reliably lock out
2253+
# the job's own account -- review, PR #246: restricted to Linux so the
2254+
# assertions below are actually exercising the locked path, not silently
2255+
# passing because both files got analyzed).
2256+
if: runner.os == 'Linux'
22522257
run: |
22532258
mkdir -p "$RUNNER_TEMP/owen-mixed-locked/locked" "$RUNNER_TEMP/owen-mixed-locked/readable"
22542259
cat > "$RUNNER_TEMP/owen-mixed-locked/locked/Secret.cs" <<'EOF'
@@ -2258,17 +2263,21 @@ jobs:
22582263
using System.IO;
22592264
public class Leaky { public void Run() { var s = new MemoryStream(); s.WriteByte(1); } }
22602265
EOF
2261-
chmod 000 "$RUNNER_TEMP/owen-mixed-locked/locked" 2>/dev/null || true
2266+
chmod 000 "$RUNNER_TEMP/owen-mixed-locked/locked"
22622267
set +e
22632268
out=$(owen check "$RUNNER_TEMP/owen-mixed-locked" --fail-on-finding 2>&1)
22642269
rc=$?
22652270
set -e
2266-
chmod 755 "$RUNNER_TEMP/owen-mixed-locked/locked" 2>/dev/null || true
2271+
chmod 755 "$RUNNER_TEMP/owen-mixed-locked/locked"
22672272
echo "$out"
22682273
if echo "$out" | grep -qi "UnauthorizedAccess\|Unhandled exception"; then
22692274
echo "FAIL: crashed on the locked subdirectory instead of tolerating it"; exit 1
22702275
fi
2271-
echo "$out" | grep -q "OWN001" || { echo "FAIL: expected the readable source's leak to still be found"; exit 1; }
2276+
[ "$rc" -eq 1 ] || { echo "FAIL: expected exit 1 (the readable source's leak), got $rc"; exit 1; }
2277+
echo "$out" | grep -q "Real.cs" || { echo "FAIL: readable source was not analyzed"; exit 1; }
2278+
if echo "$out" | grep -q "Secret.cs"; then
2279+
echo "FAIL: the inaccessible source was unexpectedly analyzed -- chmod 000 did not actually lock it out, so this run proves nothing"; exit 1
2280+
fi
22722281
- name: Uppercase extensions are accepted case-insensitively (Foo.CS)
22732282
run: |
22742283
mkdir -p "$RUNNER_TEMP/owen-uppercase"

‎docs/notes/owen-public-facade.md‎

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -79,7 +79,7 @@ Per the guardrail this rebrand was scoped to: no mass rename.
7979
language-neutral at the product level ("Owen finds lifetime and
8080
resource-contract bugs") while explicitly listing what this distribution
8181
actually wires up today:
82-
```
82+
```text
8383
Included frontend:
8484
.NET / C# (.cs, .csproj, .sln)
8585
```

‎frontend/roslyn/OwnSharp.Cli/CoreVendor.cs‎

Lines changed: 16 additions & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -116,8 +116,20 @@ public static string EnsureUnpacked()
116116
// (same fingerprint), so just use it.
117117
return finalRoot;
118118
}
119-
Directory.CreateDirectory(finalRoot);
120-
Directory.Move(tempOwnlang, finalOwnlang);
119+
try
120+
{
121+
Directory.CreateDirectory(finalRoot);
122+
Directory.Move(tempOwnlang, finalOwnlang);
123+
}
124+
catch (IOException) when (Directory.Exists(finalOwnlang))
125+
{
126+
// Narrower version of the same race (review, PR #246): a concurrent
127+
// process created finalOwnlang between the check above and this
128+
// Move. Same reasoning -- their content is provably identical
129+
// (same fingerprint), so just use it instead of surfacing the
130+
// IOException Move throws for an existing destination.
131+
return finalRoot;
132+
}
121133
return finalRoot;
122134
}
123135
finally
@@ -154,6 +166,7 @@ private static string Fingerprint(IReadOnlyList<string> files)
154166
writer.Write(contentBytes);
155167
}
156168
writer.Flush();
157-
return Convert.ToHexString(sha.ComputeHash(buffer.ToArray()));
169+
buffer.Position = 0;
170+
return Convert.ToHexString(sha.ComputeHash(buffer));
158171
}
159172
}

0 commit comments

Comments
 (0)