Skip to content

feat(cli): review and apply saved SECURITY.md drafts - #489

Open
mldangelo-oai wants to merge 67 commits into
mdangelo/codex/security-policy-draftsfrom
mdangelo/codex/security-policy-command
Open

feat(cli): review and apply saved SECURITY.md drafts#489
mldangelo-oai wants to merge 67 commits into
mdangelo/codex/security-policy-draftsfrom
mdangelo/codex/security-policy-command

Conversation

@mldangelo-oai

@mldangelo-oai mldangelo-oai commented Aug 17, 2026

Copy link
Copy Markdown
Collaborator

Summary

Add an owner-controlled way to review and apply a saved SECURITY.md draft. policy --apply loads the draft without starting Codex, validates its repository and component scope, shows the exact diff, and writes only after confirmation or an explicit --write.

This PR remains stacked on #536.

Changes

  • Load saved drafts through the CLI and exported SDK APIs.
  • Recheck the target, inherited policies, repository binding, and policy links before writing, including when the reviewed policy content is unchanged.
  • Preserve Unix owners, groups, special mode bits, access-control entries, and existing Linux security contexts alongside complete Windows security descriptors, discretionary and system access-control entries, inherited audit rules, and mandatory integrity labels.
  • Compare complete Windows discretionary and system ACL bytes, access-control flags, inheritance protection, resource attributes, scoped policies, process-trust entries, and access filters using the existing account's permissions.
  • Leave matching Windows audit descriptors untouched while copying only the access, owner, or group sections that actually need updating.
  • Keep Windows policy installation and rollback atomic without replacing a concurrent writer or exposing an intermediate file with a default security descriptor.
  • Preserve Unix security metadata through read-only installation and rollback even when hard links are unavailable, using atomic exclusive replacement without overwriting a concurrent writer.
  • Refuse to install unstable Unix or Windows rollback snapshots by bracketing complete security metadata, descriptors, and contents with independent inode change-generation checks.
  • Recheck the reviewed installed policy, its retained recovery contents, and out-of-scope policy aliases after completing permission verification on every platform.
  • Preserve open-writer recovery semantics and exercise real Windows discretionary access, audit, integrity-label, resource-attribute, inheritance, collision, rollback, and concurrent-update behavior.
  • Update documentation, installed-package smoke coverage, policy API and CLI tests, and narrowly scoped cross-platform sandbox test fixtures.

Testing

Checks on 645bd253eadd71d9c4dbfae14069604380b0d920:

  • ./node_modules/.bin/tsc --noEmit: passed.
  • ./node_modules/.bin/prettier --check src/security-policy.ts tests-ts/security-policy.test.ts: passed.
  • git show --format= --check HEAD: passed.
  • bun test --timeout 120000 tests-ts/security-policy.test.ts tests-ts/cli-policy.test.ts tests-ts/api-policy.test.ts tests-ts/config.test.ts: 199 passed, 14 platform-specific tests skipped, 0 failed.
  • A real open-writer regression changes Unix permissions or a native Windows discretionary ACL after rollback descriptor verification; recovery now rejects the unstable snapshot and retains the concurrently updated original.
  • Fresh exact-head Linux, macOS, and Windows CI validates package and policy matrices, including existing-policy security contexts, complete discretionary/system access controls, mandatory labels, and concurrent recovery.

Risk and rollout

Applying a draft replaces the selected SECURITY.md only after explicit approval. It does not stage, commit, or publish the policy. If verification fails after the write, the command reports the installed path and any recovery path for owner reconciliation. Windows descriptor operations fail closed and do not broaden sandbox or filesystem permissions or adjust token privileges. Existing Linux policy-context preservation uses only the existing account's permissions. Recovery generation checks detect concurrent content or metadata changes according to filesystem change-time precision; they do not make the final check and the subsequent operating-system move one atomic operation. This PR adds no npm dependencies.

Open review item: Filename-specific SELinux context transitions when creating a brand-new policy remain unresolved and are not changed by this update.

Merge #536 first, then retarget this PR to main and rerun exact-head validation before merge.

Public disclosure review

  • No customer, partner, prospect, or user identities, data, or identifying details are included.
  • No credentials, personal data, private source, scan findings, or nonpublic links or tickets are included.
  • I reviewed the branch name, title, description, commits, changes, comments, logs, screenshots, attachments, and links for public disclosure.

The second attestation remains unchecked because preexisting public history includes contributor metadata and automated review artifacts.

Copy link
Copy Markdown
Collaborator Author

@codex review

Please review the current head, b1de4d9394c8c8ef7a155d0246735405186c9a11.

@chatgpt-codex-connector

Copy link
Copy Markdown

Codex Review: Didn't find any major issues. 🚀

Reviewed commit: b1de4d9394

ℹ️ About Codex in GitHub

Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you

  • Open a pull request for review
  • Mark a draft as ready
  • Comment "@codex review".

If Codex has suggestions, it will comment; otherwise it will react with 👍.

Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".

@chatgpt-codex-connector chatgpt-codex-connector Bot left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

💡 Codex Security Review

Here are some automated security review suggestions for this pull request.

Reviewed commit: b1de4d9394

View security finding report

Only the user who started this review can view the report in Codex.

ℹ️ About Codex security reviews in GitHub

This is an experimental Codex feature. Security reviews are triggered when:

  • You comment "@codex security review"
  • A regular code review gets triggered (for example, "@codex review" or when a PR is opened), and you’re opted in so security review runs alongside code review

Once complete, Codex will leave suggestions, or a comment if no findings are found.

Comment thread sdk/typescript/src/security-policy.ts

Copy link
Copy Markdown
Collaborator Author

@codex review

Please review the current head, 5c0acd8.

@mldangelo-oai

Copy link
Copy Markdown
Collaborator Author

@codex review
@codex security review

Please review exact head 326af9b98801d809e913044cd3167c73ad37fa20, including Windows audit-inheritance preservation, final read-only verification of the installed policy against its retained recovery file, and the narrowly scoped cross-platform sandbox test-fixture updates.

Local TypeScript and formatting checks passed. The combined policy, CLI, API, and configuration suites passed: 189 tests passed, 8 platform-specific tests skipped, and 0 tests failed. Exact-head native Windows package and matrix checks are pending.

@chatgpt-codex-connector chatgpt-codex-connector Bot left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

💡 Codex Review

Here are some automated review suggestions for this pull request.

Reviewed commit: 326af9b988

ℹ️ About Codex in GitHub

Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you

  • Open a pull request for review
  • Mark a draft as ready
  • Comment "@codex review".

If Codex has suggestions, it will comment; otherwise it will react with 👍.

Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".

Comment thread sdk/typescript/src/security-policy.ts Outdated
Comment thread sdk/typescript/src/security-policy.ts
@mldangelo-oai

Copy link
Copy Markdown
Collaborator Author

@codex review
@codex security review

Please review exact head 8ee5fe1c43eb26f92abcefb98d8dc0e9f2378d2d, including preserving existing Windows audit descriptors without changing privileges, distinguishing inherited audit rules from explicit rules, and verifying installed permissions on every platform before reporting success.

The Unix permission-race regression failed on the preceding head and passes with this change. Local TypeScript, formatting, and combined policy, CLI, API, and configuration checks passed; exact-head native Windows package and matrix checks are pending.

@chatgpt-codex-connector chatgpt-codex-connector Bot left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

💡 Codex Review

Here are some automated review suggestions for this pull request.

Reviewed commit: 8ee5fe1c43

ℹ️ About Codex in GitHub

Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you

  • Open a pull request for review
  • Mark a draft as ready
  • Comment "@codex review".

If Codex has suggestions, it will comment; otherwise it will react with 👍.

Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".

Comment thread sdk/typescript/src/security-policy.ts
Comment thread sdk/typescript/src/security-policy.ts Outdated
Comment thread sdk/typescript/src/security-policy.ts Outdated
@mldangelo-oai

Copy link
Copy Markdown
Collaborator Author

@codex review
@codex security review

Please review exact head 95f168ba41e01fc13534285296585a07c7d8ba52, including complete Windows audit control flags and mandatory integrity labels, preserving Unix owners, groups, special permissions, and access-control entries, and revalidating retained recovery evidence when retrying an installed policy.

Real Unix permission-metadata and verification-retry regressions failed on the preceding head and now pass. Local TypeScript, formatting, and combined policy, CLI, API, and configuration checks passed; exact-head native Windows and macOS checks are pending.

@chatgpt-codex-connector chatgpt-codex-connector Bot left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

💡 Codex Review

Here are some automated review suggestions for this pull request.

Reviewed commit: 95f168ba41

ℹ️ About Codex in GitHub

Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you

  • Open a pull request for review
  • Mark a draft as ready
  • Comment "@codex review".

If Codex has suggestions, it will comment; otherwise it will react with 👍.

Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".

Comment thread sdk/typescript/src/security-policy.ts
Comment thread sdk/typescript/src/security-policy.ts
Comment thread sdk/typescript/src/security-policy.ts Outdated
Comment thread sdk/typescript/src/security-policy.ts Outdated
@mldangelo-oai

Copy link
Copy Markdown
Collaborator Author

@codex review
@codex security review

Please review exact head a47da34698991972ac7287530215199d23be4e17, including permission-preserving atomic Unix installation and rollback without hard links, safe cross-filesystem recovery retries, recovery-content revalidation, validated snapshot state, and preserving installed-policy error classification.

Real read-only and rollback access-control regressions, cross-filesystem recovery, retry races, existing alias verification, and atomic concurrent-writer protections pass locally. Exact-head native Windows and macOS checks are pending.

@chatgpt-codex-connector chatgpt-codex-connector Bot left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

💡 Codex Review

Here are some automated review suggestions for this pull request.

Reviewed commit: a47da34698

ℹ️ About Codex in GitHub

Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you

  • Open a pull request for review
  • Mark a draft as ready
  • Comment "@codex review".

If Codex has suggestions, it will comment; otherwise it will react with 👍.

Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".

Comment thread sdk/typescript/src/security-policy.ts Outdated
Comment thread sdk/typescript/src/security-policy.ts
@mldangelo-oai

Copy link
Copy Markdown
Collaborator Author

@codex review
@codex security review

Please review the exact current head afbdd0e8ffa9b74521a0266e17c75b1859fbef10, including Linux security-context preservation, stable recovery snapshots, cross-platform security metadata, unchanged-draft validation, and policy installation/recovery races.

Local validation: 196 tests passed, 11 platform-specific tests skipped, and no tests failed. Fresh Linux, macOS, and Windows CI is running.

@chatgpt-codex-connector chatgpt-codex-connector Bot left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

💡 Codex Review

Here are some automated review suggestions for this pull request.

Reviewed commit: afbdd0e8ff

ℹ️ About Codex in GitHub

Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you

  • Open a pull request for review
  • Mark a draft as ready
  • Comment "@codex review".

If Codex has suggestions, it will comment; otherwise it will react with 👍.

Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".

Comment thread sdk/typescript/src/security-policy.ts
@mldangelo-oai

Copy link
Copy Markdown
Collaborator Author

@codex review
@codex security review

Please review exact current head 5a1ed9c1c805ca51665083f9ea6d41a5985426de, including complete ordered Windows system access-control entries, resource and scoped-policy categories, mandatory integrity labels, Linux security contexts, stable recovery snapshots, and cross-platform installation/recovery races.

Local validation: 196 tests passed, 12 platform-specific tests skipped, and no tests failed. Fresh native Windows, Linux, and macOS CI is running.

@chatgpt-codex-connector chatgpt-codex-connector Bot left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

💡 Codex Review

Here are some automated review suggestions for this pull request.

Reviewed commit: 5a1ed9c1c8

ℹ️ About Codex in GitHub

Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you

  • Open a pull request for review
  • Mark a draft as ready
  • Comment "@codex review".

If Codex has suggestions, it will comment; otherwise it will react with 👍.

Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".

Comment thread sdk/typescript/src/security-policy.ts
Comment on lines +1235 to +1238
const temporary = join(
dirname(target.targetPath),
`.SECURITY.md.${randomUUID()}.tmp`,
);

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

P2 Badge Apply the target SELinux label when creating a policy

On SELinux systems with a filename-specific type transition for SECURITY.md, creating the inode under this randomized .tmp name gives it the temporary file's context, and the later hard-link or rename does not recompute the label for the final name. Because the new-policy path has no metadata reference, post-write verification never notices that the installed policy has the wrong mandatory-access context. The earlier existing-policy context preservation does not cover this branch; create or relabel the inode using the target's expected context and verify it after installation.

AGENTS.md reference: sdk/typescript/AGENTS.md:L27-L27

Useful? React with 👍 / 👎.

@mldangelo-oai

Copy link
Copy Markdown
Collaborator Author

@codex review
@codex security review

Please review exact current head 8f061f012d56d4e1b21452481dfb9dc9499394af, particularly the new stable Windows rollback snapshot verification, complete system access-control entries, resource attributes, mandatory integrity labels, existing Linux security contexts, and cross-platform installation/recovery races.

The separate open review item concerning filename-specific SELinux context transitions for a brand-new policy remains unresolved and is intentionally not changed by this Windows-only update.

Local validation: 196 tests passed, 13 platform-specific tests skipped, and no tests failed. Fresh native Windows, Linux, and macOS CI is running.

@chatgpt-codex-connector chatgpt-codex-connector Bot left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

💡 Codex Review

Here are some automated review suggestions for this pull request.

Reviewed commit: 8f061f012d

ℹ️ About Codex in GitHub

Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you

  • Open a pull request for review
  • Mark a draft as ready
  • Comment "@codex review".

If Codex has suggestions, it will comment; otherwise it will react with 👍.

Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".

Comment thread sdk/typescript/src/security-policy.ts Outdated
Comment thread sdk/typescript/src/security-policy.ts
Comment thread sdk/typescript/src/security-policy.ts
@mldangelo-oai

Copy link
Copy Markdown
Collaborator Author

@codex review
@codex security review

Please review exact current head bfac0391e2d2e851ccd7087b921a18cdf90c3279, including complete raw discretionary/system ACL bytes and control flags, stable Windows recovery snapshots after descriptor verification, reviewed installed-content rechecks, resource attributes, mandatory integrity labels, and cross-platform recovery races.

The separate open review item concerning filename-specific SELinux context transitions for a brand-new policy remains unresolved and is intentionally not changed by this update.

Local validation: 197 tests passed, 14 platform-specific tests skipped, and no tests failed. Fresh native Windows, Linux, and macOS CI is running.

@chatgpt-codex-connector chatgpt-codex-connector Bot left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

💡 Codex Review

Here are some automated review suggestions for this pull request.

Reviewed commit: bfac0391e2

ℹ️ About Codex in GitHub

Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you

  • Open a pull request for review
  • Mark a draft as ready
  • Comment "@codex review".

If Codex has suggestions, it will comment; otherwise it will react with 👍.

Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".

Comment thread sdk/typescript/src/security-policy.ts
@mldangelo-oai

Copy link
Copy Markdown
Collaborator Author

@codex review
@codex security review

Please review exact current head d9b4f0678b7753874376b9896ec8744698409921, including late out-of-scope policy aliases after permission verification, complete discretionary/system ACL bytes and control flags, stable recovery snapshots, reviewed installed-content rechecks, resource attributes, mandatory integrity labels, and cross-platform recovery races.

The separate open review item concerning filename-specific SELinux context transitions for a brand-new policy remains unresolved and is intentionally not changed by this update.

Local validation: 198 tests passed, 14 platform-specific tests skipped, and no tests failed. Fresh native Windows, Linux, and macOS CI is running.

@chatgpt-codex-connector chatgpt-codex-connector Bot left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

💡 Codex Review

Here are some automated review suggestions for this pull request.

Reviewed commit: d9b4f0678b

ℹ️ About Codex in GitHub

Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you

  • Open a pull request for review
  • Mark a draft as ready
  • Comment "@codex review".

If Codex has suggestions, it will comment; otherwise it will react with 👍.

Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".

Comment thread sdk/typescript/src/security-policy.ts
Comment thread sdk/typescript/src/security-policy.ts
@mldangelo-oai

Copy link
Copy Markdown
Collaborator Author

@codex review
@codex security review

Please review exact current head 645bd253eadd71d9c4dbfae14069604380b0d920, including independent Unix and Windows recovery change-generation checks, complete discretionary/system ACL bytes and control flags, open-writer metadata and content races, late out-of-scope policy aliases, resource attributes, and mandatory integrity labels.

The separate open review item concerning filename-specific SELinux context transitions for a brand-new policy remains unresolved and is intentionally not changed by this update. Filesystem change-generation checks do not make their final observation and the following operating-system move one atomic operation.

Local validation: 199 tests passed, 14 platform-specific tests skipped, and no tests failed. Fresh native Windows, Linux, and macOS CI is running.

@chatgpt-codex-connector

Copy link
Copy Markdown

Codex Review: Didn't find any major issues. Bravo.

Reviewed commit: 645bd253ea

ℹ️ About Codex in GitHub

Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you

  • Open a pull request for review
  • Mark a draft as ready
  • Comment "@codex review".

If Codex has suggestions, it will comment; otherwise it will react with 👍.

Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".

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

Labels

enhancement New feature or request

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants