Skip to content

security: reject unknown OAuth connection authority fields - #339

Draft
seonghobae wants to merge 105 commits into
security/private-evidence-object-bound-current-v1from
security/provider-oauth-document-schema-v1
Draft

security: reject unknown OAuth connection authority fields#339
seonghobae wants to merge 105 commits into
security/private-evidence-object-bound-current-v1from
security/provider-oauth-document-schema-v1

Conversation

@seonghobae

@seonghobae seonghobae commented Sep 3, 2026

Copy link
Copy Markdown
Contributor

Purpose

Harden DiskSage's local provider-OAuth domain authority while keeping reusable filesystem publication in #344 and shipped CLI/process/platform behavior in #212. Refresh-token secrets remain in the OS credential store; this PR owns OAuth connection-document/token/callback/credential semantics.

Current exact state — 2026-09-07 KST

Filesystem-owner adoption and replacement-unavailable consumer contract

#339 inherits #344 through ordinary non-force ancestry. A missing connection document may use canonical private create-new publication at exact Unix mode 0600; an existing regular document returns oauth-connection-document-object-bound-replacement-unavailable before staging/delete-and-create/pathname fallback. Provider-domain semantics remain here; reusable filesystem publication authority remains in #344.

Valid finding and repair — canonical publication owner

Fresh review found that provider_oauth.rs compiled src/private_directory_publication.rs a second time under #[cfg(test)] through include!, then routed unit-test writes through that duplicate module while production used crate::private_directory_publication. That made unit tests exercise a distinct module instance instead of the inherited owner boundary.

Exact source-contract RED commit dafe81757f0cb264ce76a5c8f805ed00e708239b adds provider_oauth_publication_owner_contract.rs, requiring provider OAuth to consume exactly one canonical crate::private_directory_publication::write_private_bytes_create_new_with_parents call and forbidding the test-private included owner copy. Its first Test run 34104885092 was cancelled by the causal fix push before the Ubuntu lane received a runner; the unchanged RED test job was explicitly re-requested for hosted evidence rather than treated as passing.

Minimal causal fix eaf1bc88636f573b230ccc71ca5dd4ae78aa370f removes only the test-private include! module and the cfg(test) duplicate wrapper. Compare dafe817...eaf1bc8... is one commit, one file, 16 deletions, 0 additions. Test and production now call the same crate-owned private-directory publication primitive; no filesystem behavior, OAuth contract, permissions, pathname fallback, or replacement authority was widened.

Provider-domain safeguards retained

Connection/document deserialization rejects unknown fields; duplicate IDs fail closed; connection IDs normalize macOS File Provider Unicode forms while retaining legacy lookup compatibility; loopback callbacks remain bounded and host-authoritative; refresh tokens remain in the OS credential store and outside settings/command responses. Existing-record replacement remains intentionally unavailable until exact-source-object mutation and recovery semantics are accepted.

Downstream order and remaining gaps

Owner #344 GREEN is not consumer GREEN. #339 must earn terminal exact-head Test/security/review evidence on eaf1bc8...; only then may #212 restack by ordinary non-force ancestry. Issue #342 remains open for stable publication authority, especially Windows native-handle/reparse/durability parity. Issue #170 remains open for exact-object Trash/deletion, durable journal/receipt, crash/power-loss recovery and undo. #264 remains a release prerequisite until its canonical CodeQL receipt is terminal success.

Keep Draft until the consumer exact head and prerequisites have terminal passing applicable Test/security/SAST/CodeQL/coverage/review evidence. No self-approval, gate weakening, administrative bypass, pathname fallback, predecessor-evidence transfer, source copy, force-push, destructive rebase, or premature Close.

@coderabbitai

coderabbitai Bot commented Sep 3, 2026

Copy link
Copy Markdown

Important

Draft PR not reviewed

Draft PRs are not automatically reviewed by default.

  • Trigger a manual review

To automatically review draft PRs, update your CodeRabbit configuration:

reviews:
  auto_review:
    drafts: true

Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out.

❤️ Share

Comment @coderabbitai help to get the list of available commands.

seonghobae commented Sep 6, 2026

Copy link
Copy Markdown
Contributor Author

Owner refresh — 2026-09-06 KST

Reusable filesystem owner #344 advanced non-force to exact 6f934ebf87061432a0fb89ff3237aaefb687a367. Fresh compare keeps merge base 280a0059e14374d6bbee667fb899de511c5bb311; this #339 head b0c523a822c39e6c44c4c0502e2c35abe919d930 is now behind_by=19 relative to current #344. Current owner now binds/revalidates forbidden/source-root identity and, after 39eec7f7...6f934ebf..., no longer provisions missing no-policy private ancestors through mkdirat() followed by pathname openat(). The final parent must already exist at exact 0700 and is bound/revalidated as the publication object. Exact owner Test 34027050887 is non-terminal, so this provider-domain branch intentionally remains on last terminal-GREEN owner 280a0059.... After unchanged-head #344 GREEN, adopt it by ordinary non-force ancestry, adapt any parent-preparation assumption without copying owner source, and reacquire #339's own Test/review evidence.

Copy link
Copy Markdown
Contributor Author

Owner-authority refresh, 2026-09-07 KST: current reusable filesystem owner #344 is exact 666cedae424454671e601a1117c3199e1bc2aaf9. Fresh compare keeps merge base 20177e3a92c6b3dcdd2996ab0e3fd8c5f23145be; current #344 is 18 owner commits ahead of that merge base. #344 exact Test 34047647718 is not terminal GREEN: Windows 101525500311 succeeded, while Ubuntu test 101525500336 and llm-engine-build 101525500100 remain runner-unassigned queued. Therefore keep #339 unchanged; do not transfer predecessor GREEN or restack yet. Once the unchanged current #344 earns GREEN, adopt it by ordinary non-force ancestry and reacquire #339's own exact checks before #212 moves.

@seonghobae seonghobae added bug Something isn't working priority: high High-priority or P1 work status: draft Draft pull request type: bug Defect or incorrect behavior labels Sep 6, 2026 — with ChatGPT Codex Connector

Copy link
Copy Markdown
Contributor Author

Fresh consumer repair/evidence sweep on 2026-09-07 KST:

  • predecessor exact f8ddd5444b09496c893063dc806a95d478163cea, Test 34053995024, was terminal failure only in Ubuntu test 101542443975; Windows and llm-engine-build succeeded;
  • after adopting filesystem owner security: restore object-bound private evidence publication #344, the credential-free first-publication coverage still assumed OAuth persistence could provision a missing first-use-app-data parent. Current security: restore object-bound private evidence publication #344 deliberately grants create-new record authority only beneath a pre-existing exact-private parent; missing-parent provisioning is unavailable rather than inferred from pathname authority;
  • 1bfc2796cf96dfacc60f5a7f23a8554ad3816d59 minimally adapts that consumer test: the application fixture creates its app-data parent and sets exact 0700 on Unix before save_connections, while the production writer still owns only create-new 0600 record publication. Replacement refusal, no-staging, writable-parent rejection, duplicate/count rejection, non-regular destination, and symlink-target preservation assertions remain intact;
  • exact Test 34078580933 is current. Windows 101609386406 is terminal success. Ubuntu test 101609386275 and llm-engine-build 101609386551 remain queued with runner_id=0 and no steps at this sweep, so this head is not GREEN;
  • current returned review-thread count is zero;
  • release/Test owner fix: verify Windows release artifact namespace #264 has advanced to fea9144a15234035fbdf37e9fa914e5fd51efdec. Against that owner this consumer remains behind_by=2 with merge base 3e33229b4157a084a1985a1c7e952f98a64f25e3; the two owner commits are release-verifier-only. Ordinary non-force owner restack/adoption is still required before final readiness, after this consumer earns its own exact-head evidence.

Keep Draft and keep #212 held. This repair does not broaden filesystem publication or credential-deletion authority.

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

Labels

bug Something isn't working priority: high High-priority or P1 work status: draft Draft pull request type: bug Defect or incorrect behavior

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant