Skip to content

fix(agt): create envelope, session-context, audit, and session-config files with 0600/0700 modes - #343

Open
drk-ram wants to merge 1 commit into
GenAI-Security-Project:integrationfrom
drk-ram:refimpl/log-file-modes-71
Open

drk-ram wants to merge 1 commit into
GenAI-Security-Project:integrationfrom
drk-ram:refimpl/log-file-modes-71

Conversation

@drk-ram

@drk-ram drk-ram commented Oct 28, 2026 •

Copy link
Copy Markdown
Collaborator

What changed

Sets mode: 0o700 on mkdirSync and mode: 0o600 on appendFileSync / writeFileSync across envelope-log-sink.ts, server.ts (createSessionContextLogAppender), audit-sink.ts, and session-config.ts (matching otlp-file-exporter.ts), notes in reference-implementations/agt/README.md that .acs/envelopes.jsonl holds raw tool outputs as well as raw tool arguments, and adds mode tests in envelope-log-sink.test.ts and audit-sink.test.ts.

Which issue does this implement

Part of #71

  • This is an editorial correction (typo, grammar, link, formatting) with no change in
    meaning, so it needs no issue

Base branch

  • integration, because this touches the specification, schemas, a reference
    implementation, an adapter, tests, or CI
  • main, because every changed path is on the documentation lane allowlist in
    CONTRIBUTING.md

Type of change

  • Specification change (schema, hooks, events, AgBOM)
  • Reference implementation or adapter
  • Documentation
  • Tooling or CI
  • Governance (licensing, security policy, contributor docs)

I tested this

  • I synced my branch with the base branch before opening this
  • uv run pytest -v passes on my machine
  • uv run mkdocs build --strict passes on my machine

Checklist

  • Commits are signed off with git commit -s (required by the DCO)
  • Prose follows STYLE.md
  • No secrets, tokens, or internal URLs in the diff

Security

  • This change has no security impact

Restricts newly created .acs/ directories (0700) and log/session files (0600) to the owning user so raw tool arguments and tool outputs written before redaction are not world-readable under default process umasks.

@github-actions

Copy link
Copy Markdown

This pull request's description says it closes #71, which is on the project roadmap. When it merges, GitHub closes that issue and the roadmap counts it as delivered. Please confirm the change delivers the whole issue before merging.

@rocklambros rocklambros left a comment

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Raghuvar, your modes work on a new install. I ran the real Guardian and the real host hook under umask 000, then listed .acs/. On integration 19b1197 the envelope, session and audit files were 0666 and the directories 0777. With your PR every file is 0600 and every directory is 0700, on Bun 1.3.11 and 1.3.14.

The third file from my note on issue #71 is also fine: ACS_OTEL_FILE lands 0600 and holds no copy of the key. Every suite passes on the merged tree, with 1289 Bun tests (the old 1287 plus your 2).

I need the changes below. None of them depends on the leads, so please push them now, all in one push. All paths are under reference-implementations/agt/. I ran all four in a scratch copy, and the suites pass.

  1. An old log stays open. The mode option only works when the call creates the file, so packages/guardian/src/envelope-log-sink.ts:145, packages/guardian/src/server.ts:398 and packages/host-adapter/src/audit-sink.ts:326 leave an existing file as it was. Marco Bellingeri measured his files at 644 on issue #71. I ran that case with your PR: the log stayed 0644, and the key count in it went from 1 to 4. Issue #283 already sets the rule for the OTLP file, which is to tighten and not refuse. Please use the same rule here.

    Add this above createEnvelopeLogSink in envelope-log-sink.ts, and import closeSync, fchmodSync, fstatSync and openSync:

    /**
     * Appends to a file only its owner can read. The mode on `openSync` applies
     * only when this call creates the file, so a log left by an older version
     * keeps its 0644. `fchmodSync` tightens that log too. Windows has no such
     * bits, and a device such as /dev/null is not ours to change.
     */
    export function appendPrivate(path: string, text: string): void {
      const fd = openSync(path, "a", 0o600);
      try {
        if (process.platform !== "win32" && fstatSync(fd).isFile()) {
          fchmodSync(fd, 0o600);
        }
        appendFileSync(fd, text);
      } finally {
        closeSync(fd);
      }
    }

    Call appendPrivate at line 145. In server.ts, import it from ./envelope-log-sink.ts, call it at line 398, and remove appendFileSync from line 66. In audit-sink.ts, add the same function and imports without export, and call it at line 326, because that file imports nothing from the Guardian.

    Keep your mkdirSync lines as they are, and do not chmod a directory that already exists. ACS_ENVELOPE_LOG=/tmp/x.jsonl would then try to change /tmp. Leave otlp-file-exporter.ts alone, because issue #283 covers it.

    Test: the tests in ask 2 fail without this change.

  2. server.ts:388, server.ts:398, session-config.ts:166 and session-config.ts:169 have no test. I removed the mode from each one, and all 1289 tests still passed. Your two tests also read whatever umask the shell has, so under umask 077 they cannot see a missing mode.

    In your two tests (packages/guardian/test/envelope-log-sink.test.ts:173, packages/host-adapter/test/audit-sink.test.ts:108), add const previous = process.umask(0); before try, and add process.umask(previous); as the first line of finally. Then add these lines after the last expect, and import chmodSync. In the audit test, use createAuditSink({ path }).write(EVENT) on the third line.

    // A log an older version left behind, opened by a new sink.
    chmodSync(path, 0o644);
    expect(createEnvelopeLogSink({ path }).write("request", REQUEST, "steps/toolCallRequest")).toBe(true);
    expect((statSync(path).mode & 0o777).toString(8)).toBe("600");
    expect(readFileSync(path, "utf8").trim().split("\n")).toHaveLength(2);

    After each of those tests, add this one. The audit version uses createAuditSink({ path: "/dev/null" }).write(EVENT).

    it.skipIf(process.platform === "win32")("writes to a device without changing its mode (#71)", () => {
      expect(createEnvelopeLogSink({ path: "/dev/null" }).write("request", REQUEST, "steps/toolCallRequest")).toBe(true);
    });

    Add this after the test at packages/guardian/test/server.test.ts:1364, and import chmodSync and statSync:

    it.skipIf(process.platform === "win32")("keeps the session context log private, new or old (#71)", async () => {
      const dir = join(tmpdir(), `acs-session-dir-${crypto.randomUUID()}`);
      const path = join(dir, "session-context.jsonl");
      const sessionId = crypto.randomUUID();
      const previous = process.umask(0);
      const guardian = await startGuardian({ ...baseOptions, sessionContextLog: path });
      const step = () => postStep(guardian, toolCallRequest({ session_id: sessionId, request_id: crypto.randomUUID() }));
      try {
        await step();
        expect((statSync(dir).mode & 0o777).toString(8)).toBe("700");
        expect((statSync(path).mode & 0o777).toString(8)).toBe("600");
        chmodSync(path, 0o644);
        await step();
        expect((statSync(path).mode & 0o777).toString(8)).toBe("600");
      } finally {
        process.umask(previous);
        await guardian.close();
        rmSync(dir, { recursive: true, force: true });
      }
    });

    Add this as the first test in the describe at packages/host-adapter/test/session-config.test.ts:40, and import statSync:

    it.skipIf(process.platform === "win32")("writes the file 0600 and creates its directory 0700 (#71)", () => {
      const dir = join(scratch(), "sessions");
      const path = sessionConfigPath(dir, "sess-1");
      const previous = process.umask(0);
      try {
        createFileSessionConfigStore({ dir, sessionId: "sess-1" }).set(HELLO);
        expect((statSync(dir).mode & 0o777).toString(8)).toBe("700");
        expect((statSync(path).mode & 0o777).toString(8)).toBe("600");
      } finally {
        process.umask(previous);
        unlinkSync(path);
        rmdirSync(dir);
      }
    });

    Test: bun test from reference-implementations/agt shows 1293 tests. If you undo ask 1, or remove the mode from a mkdirSync call or from session-config.ts:169, a test fails.

  3. README.md:370 promises 0600 for log files, and today that holds only for a new file. Replace everything after "Nothing is stripped or redacted," with:

    so the file holds raw tool arguments and tool outputs. The Guardian creates a new `.acs/` directory with mode `0700`, and sets the log to mode `0600` on every write. An `.acs/` directory that already exists keeps its mode. The file is gitignored. Do not commit it.
    

    Test: git grep -n 'log files with mode' reference-implementations/agt/README.md prints nothing.

  4. Please change Fixes #71 to Part of #71 in the issue section, and in your commit message too. The issue's title names two problems, and this PR answers the second one.

    I ran Marco's flow on the merged tree: the model got AWS_ACCESS_KEY_ID=[REDACTED], and params.payload.outputs[0].value in .acs/envelopes.jsonl still held the key. Your README line says the same thing, and that line is right.

    packages/guardian/README.md links issue #71 for the raw content, and README.md:305 links it for the collector's 0644 file, which this PR doesn't touch. Closing the issue would point both at a closed issue. If the leads say below that file modes are all this issue needs, you can change it back.

    Test: closing-choice stays green. I ran it on the edited description and it passes.

PR #112 rewrites the same two Guardian writes, so whichever PR lands second has to rebase. Its file-log-permissions.test.ts keeps an existing file's mode, which is the opposite of ask 1 and issue #283, so that test has to change too.

@bar-capsule @afogel, one scope question on issue #71. It's a P1, it has no assignee, and it isn't on the issue #32 list.

Can raw content stay in .acs/envelopes.jsonl at mode 0600, or must it stop reaching the disk?

These facts come from running the merged tree:

  • The key lands in three fields: arguments.command.value and raw_command on the request, and outputs[0].value on the result. The Guardian answers modify at both gates, after the raw line is already written (on integration, server.ts:577 runs before dispatch at line 598).
  • That order is deliberate, so an envelope that fails the schema stays visible. envelope-log-sink.ts:10-12 says the log does no redaction, and the Inspector reads this file.
  • The hash chain doesn't depend on the raw content, because session-context entries hold no payload.
  • File modes stop a second account. They don't stop the agent being governed, which runs as the same account. With the log at 0600, base64 -i .acs/envelopes.jsonl passed both gates and the key decoded from the output. The same command on the source file creds.env also passed, so the log isn't the only path. What the log adds is every value from every earlier session in one file.

If raw content can stay, Raghuvar can go back to Fixes #71 after these asks, and the issue's title should change. If it must stop, this PR stays Part of #71 and the rest of issue #71 needs an owner.

… files with 0600/0700 modes

Pass mode 0o700 to mkdirSync and tighten existing log files to 0o600 via
appendPrivate (openSync + fchmodSync) in envelope-log-sink.ts, server.ts
(createSessionContextLogAppender), and audit-sink.ts, plus mode 0o600 on
session-config.ts writeFileSync, and update reference-implementations/agt/README.md.

Part of GenAI-Security-Project#71

Signed-off-by: Raghuvar Karthik Durga <177662415+drk-ram@users.noreply.github.com>
@drk-ram
drk-ram force-pushed the refimpl/log-file-modes-71 branch from 2af86e4 to fea3ad1 Compare October 31, 2026 15:10
@drk-ram

drk-ram commented Oct 31, 2026

Copy link
Copy Markdown
Collaborator Author

@rocklambros Addressed all four asks in fea3ad1:

  1. Added appendPrivate(path, text) (openSync(path, 'a', 0o600) + fchmodSync(fd, 0o600) on non-Windows regular files) in envelope-log-sink.ts (exported and called by server.ts) and in audit-sink.ts.
  2. Wrapped the mode tests with process.umask(0), added existing-0644 tightening and /dev/null device-write tests to envelope-log-sink.test.ts and audit-sink.test.ts, and added the mode tests in server.test.ts (sessionContextLog) and session-config.test.ts.
  3. Updated reference-implementations/agt/README.md:370.
  4. Changed Fixes #71 to Part of #71 in both the PR description and the commit message.

@rocklambros rocklambros left a comment •

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Raghuvar, all four asks are done, and the code works. I ran your head fea3ad1 merged on integration 1b8f970.

Ask 1 works. An old 0644 log becomes 0600 on the next write, for the envelope log, the session context log and the audit log. A new install is still 0600 and 0700 under umask 000 and 022, on Bun 1.3.11 and 1.3.14.

Ask 2 works too. Your tests no longer depend on the shell's umask. I broke the code on purpose in 13 ways, and a test failed for 12 of them. The other one is the mode on openSync, which fchmodSync covers, as I expected.

Asks 3 and 4 are done, and closing-choice passes with Part of #71.

Every suite passes: pytest 924 passed and 1 skipped, and 1293 Bun tests with 0 failures on both Bun versions.

I found one limit in the code I asked you to write, and I accept it. Some file systems refuse a mode change. When that happens, the sink stops writing and prints an error, the same way it does when the disk is full.

On the host, a step under proceed is then blocked during a Guardian outage, because the host proceeds only with an audit line. I forced this failure to check it. It fails closed, so you don't need to change anything.

I'm approving this PR.

@bar-capsule @afogel, the scope question from my last review is still open: can raw content stay in .acs/envelopes.jsonl at mode 0600, or must it stop reaching the disk?

Your answer decides issue #71, and it doesn't change this code. Please answer first. This PR waits for your answer. If raw content can stay, Raghuvar changes that line to Fixes #71 first, and the issue closes with the merge. If it must stop, this PR stays Part of #71 and the rest of the issue needs an owner.

This branch has not been deployed

No deployments
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

Status: In progress

Development

Successfully merging this pull request may close these issues.

2 participants