Skip to content

fix(cli): require provider-scoped writeback tokens#325

Merged
khaliqgant merged 2 commits into
mainfrom
fix/writeback-require-provider-scope
Jun 19, 2026
Merged

fix(cli): require provider-scoped writeback tokens#325
khaliqgant merged 2 commits into
mainfrom
fix/writeback-require-provider-scope

Conversation

@khaliqgant

@khaliqgant khaliqgant commented Jun 19, 2026

Copy link
Copy Markdown
Member

Summary

  • remove the relayfile CLI writeback delegated-token whole-tree fallback (fs:write:/** / relayfile:fs:write:/**)
  • require writeback paths to derive a provider subtree before bootstrapping delegated credentials
  • keep normal provider writeback requests unchanged (fs:write:/linear/**, relayfile:fs:write:/linear/**)

Context

Completes the caller-side half of the relayauth#55 / relayauth#57 mount-scope fix. Relayauth v0.2.16 now accepts bounded provider-subtree path tokens while still rejecting whole-tree scopes, so callers must not send the global /** fallback to path-token mint.

Verification

  • go test ./cmd/relayfile-cli -run 'TestWritebackPushScopesRequireProviderSubtree|TestWritebackPushPostsBulkAndWritesAckedReceipt'
  • go test ./cmd/relayfile-cli

Review in cubic

@coderabbitai

coderabbitai Bot commented Jun 19, 2026

Copy link
Copy Markdown

Warning

Review limit reached

@khaliqgant, we couldn't start this review because you've reached your PR review rate limit.

More reviews will be available in 26 minutes and 29 seconds. Learn how PR review limits work.

Your organization has used up its prepaid credits, and credit purchases are no longer available. Enable the review add-on in the billing tab to keep reviews running — you're only billed for reviews past your plan's rate limits ($0.25/file).

⌛ How to resolve this issue?

After more reviews become available, a review can be triggered using the @coderabbitai review command as a PR comment. Alternatively, push new commits to this PR.

To avoid repeated limits, reduce automatic review volume by pausing incremental auto-reviews earlier, using label-based review opt-in, excluding WIP or generated PR titles, or requesting reviews manually when the PR is ready. If your team needs uninterrupted high-volume reviews, an organization admin can enable usage-based credits.

🚦 How do rate limits work?

CodeRabbit enforces per-developer PR review limits for each organization. Most developers receive the normal plan refill rate.

For paid Pro and Pro+ PR reviews, CodeRabbit uses adaptive limits for sustained high-volume activity. When a developer's recent PR review activity reaches the 95th percentile or higher among CodeRabbit users, the refill rate gradually slows as usage increases. The highest same-day bursts are limited more strictly.

Please see our Fair Usage Limits Policy for further information.

ℹ️ Review info
⚙️ Run configuration

Configuration used: Organization UI

Review profile: CHILL

Plan: Pro Plus

Run ID: 190cfb35-3188-4bf7-b7e1-3b3d9ba9a2fd

📥 Commits

Reviewing files that changed from the base of the PR and between 0bd81d5 and 2b09882.

📒 Files selected for processing (4)
  • cmd/relayfile-cli/main.go
  • cmd/relayfile-cli/main_test.go
  • internal/httpapi/server.go
  • internal/httpapi/server_test.go
✨ Finishing Touches
🧪 Generate unit tests (beta)
  • Create PR with unit tests
  • Commit unit tests in branch fix/writeback-require-provider-scope

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 and usage tips.

@gemini-code-assist gemini-code-assist 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.

Code Review

This pull request refactors the scope resolution logic for writeback push operations in cmd/relayfile-cli/main.go by introducing writebackPushScopes and a helper writebackPushProvider, along with corresponding unit tests. The feedback suggests removing a redundant length check on the output of strings.Split and deleting the now-unused functions writebackPushJoinScopes and writebackPushRequiredRelayfileScopes to clean up dead code.

Important

The consumer version of Gemini Code Assist on GitHub is being sunset. Starting June 18, 2026, new organization installations will be blocked, and all code review activity will officially cease on July 17, 2026.
For more details on the timeline and next steps, please review the Help Documentation.

Comment thread cmd/relayfile-cli/main.go Outdated
Comment on lines +3675 to +3682
if len(parts) == 0 {
return "", false
}
provider := strings.TrimSpace(parts[0])
return []string{fmt.Sprintf("relayfile:fs:write:/%s/**", provider)}
if provider == "" {
return "", false
}
return provider, true

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

medium

The check len(parts) == 0 is redundant because strings.Split always returns a slice of at least length 1 (even when splitting an empty string). We can safely remove this check to simplify the function.

	provider := strings.TrimSpace(parts[0])
	if provider == "" {
		return "", false
	}
	return provider, true

Comment thread cmd/relayfile-cli/main.go Outdated
Comment on lines 3658 to 3661
provider, ok := writebackPushProvider(remotePath)
if !ok {
return nil
}

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

medium

The function writebackPushJoinScopes is no longer used anywhere in the codebase, as its only caller in runWritebackFileMutation was updated to use writebackPushScopes. It should be completely removed to clean up dead code.

Comment thread cmd/relayfile-cli/main.go Outdated
Comment on lines +3666 to +3671
provider, ok := writebackPushProvider(remotePath)
if !ok {
return nil
}
return []string{fmt.Sprintf("relayfile:fs:write:/%s/**", provider)}
}

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

medium

The function writebackPushRequiredRelayfileScopes is no longer used anywhere in the codebase, as its only caller in runWritebackFileMutation was updated to use writebackPushScopes. It should be completely removed to clean up dead code.

@github-actions

github-actions Bot commented Jun 19, 2026

Copy link
Copy Markdown

Relayfile Eval Review

Run: .relayfile/evals/runs/2026-06-19T11-19-23-484Z-HEAD-provider
Mode: provider
Git SHA: f247a1b

Passed: 4 | Needs human: 0 | Reviewable: 0 | Missing output: 0 | Failed: 0 | Skipped: 0

Human Review Cases

No reviewable human-review cases captured Relayfile output.

@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: 0fa92f827e

ℹ️ 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 cmd/relayfile-cli/main.go
Comment on lines +3647 to +3648
return []string{fmt.Sprintf("fs:write:/%s/**", provider), "ops:read"},
[]string{fmt.Sprintf("relayfile:fs:write:/%s/**", provider)},

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

P1 Badge Enforce bulk path scopes before relying on provider tokens

When the CLI mints a delegated token for /linear/** here, it immediately uses that bearer against /fs/bulk; however I checked internal/httpapi/server.go and the scopePath switch has no bulk_write case, so authorizeBearer falls back to scopeMatches, where any relayfile:fs:write:<path> grant satisfies plain fs:write, and handleBulkWrite only checks permission policy rather than scopeMatchesPath per file. In a default-open workspace, a token minted by this path for one provider is still accepted for bulk writes to other paths if the credential is reused or exposed, so the provider-scoped writeback boundary this change depends on is not actually enforced.

Useful? React with 👍 / 👎.

@khaliqgant khaliqgant merged commit 98e6184 into main Jun 19, 2026
9 checks passed
@khaliqgant khaliqgant deleted the fix/writeback-require-provider-scope branch June 19, 2026 11:22
@agent-relay-code

Copy link
Copy Markdown
Contributor

Review: PR #325fix/writeback-require-provider-scope

Summary of the change

Two coordinated security-hardening changes that tighten writeback authorization to be provider-scoped:

  1. CLI (cmd/relayfile-cli/main.go) — Replaces writebackPushJoinScopes / writebackPushRequiredRelayfileScopes with a single writebackPushScopes that returns an error instead of falling back to whole-tree wildcard scopes (fs:write:/**, relayfile:fs:write:/**) when the remote path has no valid provider subtree. A new writebackPushProvider helper extracts and validates the provider segment via the existing validateLocalProviderID.

  2. Server (internal/httpapi/server.go) — Adds a per-file scopeMatchesPath(claims.Scopes, "fs:write", path) check at the top of the handleBulkWrite loop, rejecting out-of-scope files with a forbidden / "file access denied by path scope" BulkWriteError while still writing in-scope files (partial 202).

Assessment

The change is correct, minimal, and fail-closed in the right direction:

  • The route-level authorize for bulk_write passes scopePath = "" (server.go:258-268 has no bulk_write case), so it only verifies the token holds some fs:write scope via scopeMatches. A token scoped to relayfile:fs:write:/linear/** previously reached handleBulkWrite and could write /github/.... The new per-file scopeMatchesPath closes that gap.
  • The new server check mirrors the existing precedent in validateForkCommitEntries (server.go:1369), using the identical pattern — consistent, not a novel mechanism.
  • Broad tokens are unaffected: scopeMatchesPath returns true for *-path grants (auth.go:456-457), so legitimate workspace-wide/fs:write:/** callers keep working.
  • The CLI change turns a silent wildcard fallback into an explicit error — a tightening (fail-closed), not a fail-open regression.

This is a safety-critical area (authz), but the PR strengthens guards rather than weakening any default, so it is appropriate to merge once human-reviewed.

Verification

  • go build ./... — pass
  • go vet ./cmd/relayfile-cli/... ./internal/httpapi/... — clean
  • go test ./... (full suite, as CI runs) — all packages ok, including the two new tests
  • scripts/check-contract-surface.sh — "contract check passed" (no OpenAPI drift; BulkWriteError.code is free-form string, forbidden already used in this handler)
  • Initial cmd/relayfile-cli failures were sandbox-only (missing sleep/ps binaries); they pass with coreutils on PATH.

No files were edited — nothing mechanical to fix and no semantic changes warranted.

Addressed comments

  • No bot or human review comments are present in .workforce/context.json or the workforce directory, so there are no review threads to account for.

Advisory Notes

  • None. The diff is self-contained to its stated purpose (provider-scoped writeback enforcement).

The PR builds, passes the full Go test suite and contract check, has no merge conflicts indicated, and the remaining merge decision (an authz hardening change) requires human judgment.

@agent-relay-code

Copy link
Copy Markdown
Contributor

ℹ️ pr-reviewer: review only — no file changes were applied to the PR (nothing to commit after review). The notes below are advisory and were not pushed.

Review: PR #325fix/writeback-require-provider-scope

Summary

This PR tightens writeback authorization in two fail-closed ways:

  1. CLI (cmd/relayfile-cli/main.go): replaces writebackPushJoinScopes/writebackPushRequiredRelayfileScopes with a single writebackPushScopes that requires a provider-scoped remote path (≥2 path segments + a valid provider ID via validateLocalProviderID). Paths without a real provider segment now error instead of falling back to whole-tree wildcard scopes (fs:write:/**).
  2. Server (internal/httpapi/server.go): handleBulkWrite now performs a per-file fs:write path-scope check, emitting a forbidden BulkWriteError and skipping out-of-scope files — mirroring the existing per-entry pattern at server.go:1369.

Assessment

The change is a security tightening (narrows broad grants, adds per-file enforcement). Critically, it does not convert any fail-closed state to fail-open:

  • The CLI now returns an error where it previously returned permissive whole-tree scopes — runWritebackFileMutation (main.go:3195) propagates that error and aborts before requesting credentials.
  • The server adds a deny path; it does not widen any existing grant.

Traced impact:

  • Old functions writebackPushJoinScopes / writebackPushRequiredRelayfileScopes have no remaining callers (only writebackPushScopes is referenced). No dangling references.
  • claims.Scopes is map[string]struct{} (auth.go:43), matching scopeMatchesPath's signature — type-correct.
  • The new per-file check is consistent with the documented contract at server.go:1250 ("ACL is checked per-file inside handleBulkWrite").

Verification (run end-to-end with Go 1.22.5, matching CI)

  • go build ./... — OK
  • go test ./... — full suite passes (no FAIL/panic)
  • go test ./internal/httpapi/ -run TestBulkWrite — passes incl. new TestBulkWriteEndpointEnforcesPathScopedTokenPerFile
  • go test ./cmd/relayfile-cli/ -run TestWritebackPushScopes — passes
  • go vet on changed packages — OK
  • gofmt -l on changed files — clean
  • scripts/check-contract-surface.sh — "contract check passed" (no OpenAPI drift)

Auto-applied fixes

None. The code is correct as written and contains no lint/format/typo issues.

Addressed comments

  • No bot or human review comments were present in the provided PR metadata (.workforce/context.json) and no review-comment artifacts exist in .workforce/. Nothing to address.

Advisory Notes

  • None. The change is in scope (writeback provider-scoping), and I found no out-of-scope cleanup worth recommending.

I cannot confirm the live CI status, mergeability, or that all required checks have completed/passed from this sandbox — those are reported by cloud post-run. Because I cannot verify that every required check is green and non-pending and that the PR is mergeable, I am not printing READY.

@agent-relay-code

Copy link
Copy Markdown
Contributor

Review: PR #325fix/writeback-require-provider-scope

Summary

The PR tightens writeback authorization in two independent but aligned ways:

  1. CLI (cmd/relayfile-cli/main.go): Replaces the two helpers writebackPushJoinScopes/writebackPushRequiredRelayfileScopes with writebackPushScopes + writebackPushProvider. The new code requires the remote path to have at least two segments and a valid provider ID (validateLocalProviderID), returning an error instead of falling back to the broad whole-tree scopes fs:write:/** / relayfile:fs:write:/**. This is a deliberate fail-closed change — it removes the previous broad-wildcard credential request.

  2. Server (internal/httpapi/server.go): handleBulkWrite now runs a per-file scopeMatchesPath(claims.Scopes, "fs:write", path) check before processing each file, returning a forbidden BulkWriteError for out-of-scope paths while still writing in-scope ones (partial success → 202).

Verification

  • go build ./... — passes.
  • go test ./internal/httpapi/... — passes (incl. new TestBulkWriteEndpointEnforcesPathScopedTokenPerFile).
  • go test ./cmd/relayfile-cli/... — passes (incl. new TestWritebackPushScopesRequireProviderSubtree).
  • gofmt -l on all four changed files — clean.
  • scripts/check-contract-surface.sh — passes (no contract surface changes; BulkWriteError.code is a free-form string in the spec, so the new forbidden code introduces no drift).
  • Traced removed functions writebackPushJoinScopes/writebackPushRequiredRelayfileScopes — no remaining references anywhere.
  • Confirmed the server bulk-write check matches the existing path-scope enforcement pattern already used in validateForkCommitEntries (server.go:1369) and githubTarballWritePermissionError (github_tarball.go:611). This closes a real gap: /fs/bulk was the one write path that did not enforce per-file fs:write path scope.

Findings

No blocking issues. Both changes correctly tighten authorization (fail-closed) and are well covered by the added tests. No auto-edits were needed — the working tree is already clean and formatted.

One behavioral note worth flagging for the human (not a defect): for writeback push, the canonical-path guard is intentionally skipped, so a push to a sub-provider-depth path (e.g. /foo.json) that previously received fallback fs:write:/** scopes now returns "writeback requires a provider-scoped remote path". This is the intended effect of the PR and is exercised by the negative cases in TestWritebackPushScopesRequireProviderSubtree. I did not weaken it.

Addressed comments

  • No bot or reviewer comments were present in .workforce/context.json and there are no comment artifacts in .workforce/. Nothing to address.

Advisory Notes

  • None. The change is self-contained and within the PR's stated purpose.

The PR builds, all changed-package tests and the contract check pass, gofmt is clean, and the changes are correct fail-closed authorization tightenings. I could not verify CI check status, merge-conflict/mergeable state, or post any GitHub review from this sandbox, so I am not asserting the PR is fully green and ready for a human merge decision.

@agent-relay-code

Copy link
Copy Markdown
Contributor

One last correctness consideration: the per-file scope check is placed before the s.readFile/permission check, and uses continue to skip out-of-scope files while still processing allowed ones. The allowed files still flow into s.store.BulkWrite, which is the existing digest-emitting path — so the digest contract is unaffected (this PR only narrows what gets written, it doesn't change the write/event path). The fail-closed behavior (deny → forbidden error, file not written) is correct and matches the existing validateForkCommitEntries pattern. No fail-open regression.

The PR is sound. Writing the final review.

Review: PR #325fix/writeback-require-provider-scope

Summary

This PR closes a real authorization gap and tightens CLI scope requests. Two coordinated changes:

  1. internal/httpapi/server.go (handleBulkWrite) — adds a per-file fs:write path-scope check before each bulk-write file is processed. Previously the route-level gate only verified the token held some fs:write grant (bulk has no single scopePath, so authorizeBearer uses scopeMatches, not scopeMatchesPath), letting a /linear/**-scoped token write to /github/... in a bulk batch. The new check denies out-of-scope files with a forbidden BulkWriteError while still writing in-scope files. This mirrors the existing validateForkCommitEntries pattern (server.go:1369).
  2. cmd/relayfile-cli/main.go — replaces writebackPushJoinScopes / writebackPushRequiredRelayfileScopes with writebackPushScopes + writebackPushProvider, which now require a provider-scoped path (≥2 segments, valid provider id) and return an error instead of silently falling back to whole-tree /** scopes.

Correctness verification

  • The single caller (main.go:3195) was updated to the new 3-return signature; the two removed functions have no remaining references.
  • writebackPushProvider correctly rejects all the negative test inputs (/linear single-segment, /./record.json and /../record.json via validateLocalProviderID rejecting ., /bad provider/... via the space). It lowercases the provider, matching the fs:write:/github/** expectation for /GitHub/....
  • scopeMatchesPath / scopePathMatches confirm relayfile:fs:write:/linear/** matches /linear/issues/ISS-1.json and denies /github/issues/1.json — the server test expectations are accurate.
  • claims.Scopes is map[string]struct{}, matching scopeMatchesPath's signature — types check out.

Safety review

  • The bulk-write change is fail-closed: unmatched scope → forbidden + file skipped (not written). No fail-open regression.
  • The CLI change replaces a permissive whole-tree fallback with a hard error — strictly tightening, consistent with the prior commits on this branch.
  • Allowed files still flow through s.store.BulkWrite, so the digest/event contract (CLAUDE.md) is unchanged — this PR only narrows what is written, not the write/event path. The PR adds tests on both paths.
  • No lifecycle/reaper/dispatch/broker-ownership code touched.

Verification performed (local, CI-equivalent)

Installed Go 1.22.12 (CI uses 1.22) and ran the canonical commands:

  • go build ./... → pass
  • go test ./...all packages pass, including cmd/relayfile-cli (167s) and internal/httpapi (the two new tests)
  • ./scripts/check-contract-surface.shcontract check passed (no OpenAPI drift; no new handlers/paths/status codes — forbidden is an in-body BulkWriteError, and 202 already existed)
  • go vet ./cmd/relayfile-cli/... ./internal/httpapi/... → pass

No files were edited; the PR is mechanically and semantically clean and needed no fixes.

Addressed comments

  • No bot or reviewer comments were present in the harness inputs (.workforce/context.json has no review/comment data, and there is no comments file in .workforce/). Nothing to address.

Advisory Notes

  • None. The change is correctly scoped to its stated purpose (provider-scoped writeback enforcement).

I cannot confirm the live GitHub PR state (other CI checks such as E2E/SDK typecheck, merge-conflict/mergeable status, or any human review) from this sandbox — those are reported by cloud post-run. The Go build/test and contract checks affected by this diff pass locally, and the change requires no further code action from me.

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

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant