Repository navigation
fix(agt): support Windows UCRT/Win32 OPA path publishing and repo-relative path normalization - #337
Conversation
f144229 to
abdbd4a
Compare
rocklambros
left a comment
There was a problem hiding this comment.
Raghuvar, your backslash change works on the message in issue #336. I ran your pattern on that message with a C:\ root, and it gives specification/v0.1.0/. On integration 19b1197 the backslashes stay. Every suite passes on the merged tree with the same counts as integration, including 1287 Bun tests.
The win32 block is unverified. Linux and macOS can't load ucrtbase.dll, and CI uses ubuntu-latest only, so nobody ran this code in this review. I ran its logic on macOS with a fake bun:ffi, and that shows the logic only. It shows nothing about Windows.
Your report in issue #336 is the only Windows evidence so far.
This PR carries the win32 block, and PR #344 deletes it (ask 4 on PR #344). This PR is the one about Windows. PR #344 is a security fix that waits on a lead answer, and Windows support should not wait with it.
The two blocks are byte-identical, and the merge gives one block. PR #337 needs to land before PR #344, because PR #344 closes issue #72 and issue #72 names this Windows defect.
I need the changes below. Paths are under reference-implementations/agt/packages/. I ran asks 1 to 3 in a scratch copy, and the suites pass with them in.
-
guardian/src/server.ts:147-150lets the repo path through. The new group[^'"\\s]*reads up to the next space or quote, so it also takes a second copy of the root, and thegflag never sees that copy. The messagesearch path <root>/a:<root>/bcomes back assearch path a:<root>/b. On integration it comes back assearch path a:b. A colon, comma, semicolon, equals sign or backtick between the paths all leak the same way. This function exists to keep that path out of client errors and the envelope log. I found no throw site today that puts two paths in one token, so nothing leaks this way yet. The fix is small, so I want it in. The comment at lines 137-146 also describes a lookahead that is gone. Replace lines 137-150 with:// The root must be followed by a path separator or the end of the string. // Without that rule, a *sibling* directory whose name merely extends the root // (`<root>_old`) matched too: the literal text of REPO_ROOT is a prefix of // that name, so it stripped, leaving a misleading // `_old/reference-implementations/agt` behind -- not a disclosure of this // tree's own location, since it's a different directory entirely, but a // diagnostic that then reads as if it were one. A message naming the sibling // is left alone, in full. // // The group captures the repository-relative remainder, so the replacer in // toRepoRelativeMessage can change its backslashes to slashes. The group stops // before the next copy of the root, because a list such as `<root>/a:<root>/b` // is one token, and the `g` flag must still find the second copy. const REPO_ROOT_SOURCE = REPO_ROOT.replace(/[.*+?^${}()|[\]\\]/g, "\\$&"); const REPO_ROOT_PATTERN = new RegExp(`${REPO_ROOT_SOURCE}(?:[/\\\\]((?:(?!${REPO_ROOT_SOURCE})[^'"\\s])*)|$)`, "g");
Test: ask 2.
-
No test covers your change to this function on Linux or macOS. I removed the backslash replace, and all tests still passed. I put integration's pattern back, and all tests still passed. Add this test directly under line 961 of
guardian/test/server.test.ts:it("strips the root from every path in one token, and writes the remainder with slashes", () => { const root = fileURLToPath(new URL("../../../../../", import.meta.url)).replace(/[/\\]+$/, ""); expect(toRepoRelativeMessage(`search path ${root}/a:${root}/b`)).toBe("search path a:b"); expect(toRepoRelativeMessage(`tried ${root}/a,${root}/b;${root}/c`)).toBe("tried a,b;c"); expect(toRepoRelativeMessage(`scandir '${root}\\specification\\v0.1.0\\'`)).toBe("scandir 'specification/v0.1.0/'"); });
Test: run
bun test packages/guardian/test/server.test.tsfromreference-implementations/agt. It fails on your PR today, and it passes with ask 1. -
The five
keepLoopbackOffProxytests atguardian/test/otel-trace-exporter.test.ts:1491moved to plain objects, and neither this PR nor issue #336 says why. The change is right.process.envignores case on Windows, soNO_PROXYandno_proxyare one variable there, and the old assertions could never pass. Coverage held too: I brokekeepLoopbackOffProxyfive ways, and your tests and the old tests failed in the same five. A future reader can't see why the tests moved, so add this above line 1491:// These tests pass a plain object, not process.env. On Windows process.env // ignores case, so NO_PROXY and no_proxy are one variable there, and a test // that one exists without the other could never pass.
Test:
git grep -n "ignores case" reference-implementations/agt/packages/guardian/test/otel-trace-exporter.test.tsprints the new line. -
Your closing lines are in the right form.
Part of #92leaves issue #92 open andFixes #336closes issue #336.tools/closing_choice.pypasses them, and GitHub links only issue #336. Please add two lines to the issue section, so the PR names the accepted issues it feeds:Part of #72 Part of #110Test:
closing-choicestays green, andgh pr view 337 --json closingIssuesReferencesstill lists only issue #336. -
Please run
bun teston Windows fromreference-implementations/agton your final head, with Bun 1.3.11 (CI's pin) and with Bun 1.4.4 (the version in issue #336). You need both, because the two tests you changed to.hrefskip below Bun 1.3.14. Paste the pass, skip and fail counts and the names of the failing tests. Issue #110 lists other Windows failures, so I don't expect zero.Then please try one more thing. Bun's source says a
process.envwrite on Windows callsSetEnvironmentVariableW(src/bun.js/node/node_process.zig:345atbun-v1.3.11). So the existing line atagt-bridge/src/opa-path.ts:106may already do the job of this block. I read this and did not run it. Delete lines 111-138, add this line directly under line 81, and runbun testagain on both versions:if (process.platform === "win32") process.env[OPA_PATH_ENV] = resolved;
If the counts match, please keep this one line, and this PR then needs no Windows FFI code.
Test: the two sets of counts.
@bar-capsule @afogel, this PR needs one triage answer from you on issue #336. It has no status:accepted, no scope: label and no milestone, so this PR can't close it yet. Two of its three defects are already in accepted issue #110: the win32 C library failure and the backslash path (R7 there). The third is the .pathname problem from R6, in a file issue #110 does not list. Please do one of these:
- Accept issue #336, with labels and a milestone.
Fixes #336stays. - Close it as a duplicate of issue #110.
Fixes #336goes, and this PR staysPart of #110.
Either answer settles the closing line. The PR then still needs a Windows run by someone other than the author.
@Premmethuku75, you measured Windows 11 in issue #110. If you still have that machine, a bun test on this PR's final head would be that run.
abdbd4a to
0944df8
Compare
|
@rocklambros Addressed all five asks in
|
rocklambros
left a comment
There was a problem hiding this comment.
Raghuvar, all five asks are done. Your Windows run answered the question I could not run here, so thank you for that.
I ran your head 0944df8 merged on integration 1b8f970. A second copy of the root in one token no longer leaks. I broke the pattern on purpose in 6 ways, and a test failed each time. Asks 3 and 4 are in the PR.
For ask 5, the one line replaced 28 lines that nobody here could run. Your counts are slightly better with it: 1171 pass and 105 fail on Bun 1.4.4, against 1170 and 106. Those counts are your own report. I can't run Windows here, so this PR still needs a Windows run by someone other than you.
Every suite passes on macOS: pytest 924 passed and 1 skipped, and 1288 Bun tests with 0 failures on Bun 1.3.11, 1.3.14 and 1.4.4.
I need two small changes. The path below is under reference-implementations/agt/.
-
packages/agt-bridge/src/opa-path.ts:79: the header of this file says a Bunprocess.envwrite changes nothing the native side can see. That is true on Linux and macOS, and your new line depends on Windows being different. A future reader will think the line is a bug. Add this above line 79:// On Windows a Bun `process.env` write calls SetEnvironmentVariableW, so it // reaches the real environment and this platform needs no C library. Linux // and macOS keep the JavaScript-side copy the header describes.
Test:
git grep -n SetEnvironmentVariableW reference-implementations/agt/packages/agt-bridge/src/opa-path.tsprints the new line. -
The first paragraph of the PR description still says
ucrtbase.dll,_putenv_sandSetEnvironmentVariableA, and that code is gone. Replace those words with "aprocess.envwrite, which reaches the real Windows environment under Bun".Test:
gh pr view 337 --json bodyno longer printsucrtbase.
This PR and PR #344 now change the same line of opa-path.ts, so they conflict. My review of PR #344 has the fix, and that PR takes the rebase, so you don't need to change anything here for it.
@bar-capsule @afogel, this PR still needs the triage answer from my last review. Please accept issue #336 with labels and a milestone, or close it as a duplicate of issue #110. Either answer settles the closing line.
…normalization Publish ACS_OPA_PATH on Windows via process.env[OPA_PATH_ENV] = resolved (which calls SetEnvironmentVariableW in Bun on Windows) in packages/agt-bridge/src/opa-path.ts, normalize stripped repo-relative path separators and multi-path tokens in toRepoRelativeMessage, and use file URL hrefs for child-process dynamic imports in otel-trace-exporter.test.ts. Part of GenAI-Security-Project#72 Part of GenAI-Security-Project#92 Part of GenAI-Security-Project#110 Fixes GenAI-Security-Project#336 Signed-off-by: Raghuvar Karthik Durga <177662415+drk-ram@users.noreply.github.com>
0944df8 to
82141a4
Compare
|
@rocklambros Addressed both asks in |
Split on the repository root instead of matching the whole token, so a long token cannot end the match and hand the root back unredacted, and a root followed by a quote, colon, comma, space, or newline is stripped. On Windows, match the root in any case and in the forward-slash, doubled-backslash, /C:/, file:/// and %20 spellings. Rewrite backslashes only in the path remainder and only for a Windows root, so a regex after the path or another drive's path keeps its text, and a POSIX file name holding a backslash stays the same file. A space stays in the remainder when a quote closes the path. Return early on win32 so no libc probe runs there, and say in the code and the bridge README that Windows has no readback and that a lost write denies every evaluation on policy_invocation_failed. Use fileURLToPath in the two remaining tests that read URL.pathname. Part of GenAI-Security-Project#110 Signed-off-by: rocklambros <rock@rockcyber.com>
|
Raghuvar, both of your last asks are done, and they work. The review after that found a bug that came from my own ask. The pattern I told you to use stops matching on a very long token, and then the absolute path leaks with no error. That's on me, so I fixed it and pushed What changed:
All 48 deliberate breaks were caught, and every suite passes on the merged tree, except Two gaps remain. Other percent-encoded characters are not covered, and a path with a space and no quotes stops at the space. @Premmethuku75, could you run this on Windows, on Bun 1.3.11 and 1.4.4? From Nobody except Raghuvar has run this on Windows yet, and the PR needs that before it merges. I pushed last, so my approval doesn't count here. @bar-capsule @afogel, one of you needs to approve this one. |
|
@rocklambros Pulled 1. Bun 1.3.11 (
|
What changed
Publishes
ACS_OPA_PATHon Windows (win32) inreference-implementations/agt/packages/agt-bridge/src/opa-path.tsvia aprocess.envwrite, which reaches the real Windows environment under Bun so the bundledagent-control-specification-opa-win32-x64binary works under Bun without pre-exportingACS_OPA_PATH, normalizes backslashes in the repository-relative path remainder stripped bytoRepoRelativeMessageinpackages/guardian/src/server.ts, and uses URL.hrefand plain environment maps inpackages/guardian/test/otel-trace-exporter.test.ts.Which issue does this implement
Part of #72
Part of #92
Part of #110
Fixes #336
meaning, so it needs no issue
Base branch
integration, because this touches the specification, schemas, a referenceimplementation, an adapter, tests, or CI
main, because every changed path is on the documentation lane allowlist inCONTRIBUTING.md
Type of change
I tested this
uv run pytest -vpasses on my machineuv run mkdocs build --strictpasses on my machineChecklist
git commit -s(required by the DCO)Security