Skip to content

Preserve trusted legacy Stack exchanges - #145

Merged
lawrencecchen merged 1 commit into
mainfrom
fix/legacy-stack-capabilities
Aug 4, 2026
Merged

lawrencecchen merged 1 commit into
mainfrom
fix/legacy-stack-capabilities

Conversation

@lawrencecchen

@lawrencecchen lawrencecchen commented Aug 4, 2026 •

Copy link
Copy Markdown
Contributor

Accepts an omitted capability list only after authenticating the trusted Stack control-plane credential, preserving the pre-capability broker during rollout. Explicit capability-scoped exchanges remain unchanged. Adds coverage proving the compatibility exchange receives use and account-management scope.


View with [code]smith Autofix with [code]smith
Need help on this PR? Tag @codesmith-bot with what you need. Autofix is disabled.


Summary by cubic

Preserves legacy trusted Stack exchanges by allowing omitted capability lists only after verifying the control-plane token. For trusted calls without capabilities, we default to manage_accounts and use; explicit capability-scoped exchanges are unchanged.

  • Bug Fixes
    • Authenticate X-Subrouter-Stack-Control-Token before capability parsing to block untrusted fallbacks.
    • Default to ["manage_accounts", "use"] when capabilities are omitted on trusted requests; added tests to confirm the returned capability set.

Written for commit 4a48c8c. Summary will update on new commits.

Review in cubic

Summary by CodeRabbit

  • Bug Fixes
    • Improved Stack tenant authentication by validating trusted access before requested capabilities.
    • Login requests without specified capabilities now receive the default manage_accounts and use capabilities.
    • Explicit capability requests continue to be validated and deduplicated.

@coderabbitai

coderabbitai Bot commented Aug 4, 2026 •

Copy link
Copy Markdown

Review Change Stack

📝 Walkthrough

Walkthrough

Stack tenant exchanges now validate the trusted control token before capabilities. Empty capability input defaults to manage_accounts and use. Tests verify the default response.

Changes

Stack tenant capability handling

Layer / File(s) Summary
Authentication and capability resolution
internal/proxy/multitenant.go
The exchange validates the trusted control token before processing capabilities. Empty input defaults to manage_accounts and use; explicit capabilities retain validation.
Stack exchange validation
internal/proxy/multitenant_test.go
Tests omit explicit capabilities and verify both default capabilities in the response. An existing whoami request was reformatted without behavior changes.

Estimated code review effort: 2 (Simple) | ~10 minutes

Possibly related PRs

🚥 Pre-merge checks | ✅ 5
✅ Passed checks (5 passed)
Check name Status Explanation
Description Check ✅ Passed Check skipped - CodeRabbit’s high-level summary is enabled.
Title check ✅ Passed The title clearly describes preserving trusted legacy Stack exchanges, which is the main purpose of the changes.
Docstring Coverage ✅ Passed No functions found in the changed files to evaluate docstring coverage. Skipping docstring coverage check.
Linked Issues check ✅ Passed Check skipped because no linked issues were found for this pull request.
Out of Scope Changes check ✅ Passed Check skipped because no linked issues were found for this pull request.
✨ Finishing Touches
📝 Generate docstrings
  • Create stacked PR
  • Commit on current branch
🧪 Generate unit tests (beta)
  • Create PR with unit tests
  • Commit unit tests in branch fix/legacy-stack-capabilities

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.

@lawrencecchen
lawrencecchen merged commit 39d6565 into main Aug 4, 2026
6 of 7 checks passed
@lawrencecchen
lawrencecchen deleted the fix/legacy-stack-capabilities branch August 4, 2026 07:50

@coderabbitai coderabbitai 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.

🧹 Nitpick comments (1)
internal/proxy/multitenant_test.go (1)

528-528: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick win

Add a negative regression test for the capability fallback.

The positive case verifies the default capabilities for an authenticated exchange. Add a second request with the same body and Authorization header, but without X-Subrouter-Stack-Control-Token. Assert http.StatusUnauthorized.

The PR objective requires omitted capability lists to remain gated by the trusted Stack control-plane credential.

Example
+	unauthenticated := httptest.NewRequest(
+		http.MethodPost,
+		"/_subrouter/auth/stack",
+		strings.NewReader(`{"teamId":"team-123","teamName":"Acme"}`),
+	)
+	unauthenticated.Header.Set("Authorization", "Bearer stack-access")
+	unauthenticatedResponse := httptest.NewRecorder()
+	handler.ServeHTTP(unauthenticatedResponse, unauthenticated)
+	if unauthenticatedResponse.Code != http.StatusUnauthorized {
+		t.Fatalf("unauthenticated exchange status = %d", unauthenticatedResponse.Code)
+	}

Also applies to: 558-563

🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.

In `@internal/proxy/multitenant_test.go` at line 528, Add a negative regression
case alongside the authenticated exchange test using the same request body and
Authorization header but omitting X-Subrouter-Stack-Control-Token. Send it
through the existing handler/test flow and assert that the response status is
http.StatusUnauthorized, preserving the positive default-capabilities case.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.

Nitpick comments:
In `@internal/proxy/multitenant_test.go`:
- Line 528: Add a negative regression case alongside the authenticated exchange
test using the same request body and Authorization header but omitting
X-Subrouter-Stack-Control-Token. Send it through the existing handler/test flow
and assert that the response status is http.StatusUnauthorized, preserving the
positive default-capabilities case.

ℹ️ Review info
⚙️ Run configuration

Configuration used: defaults

Review profile: CHILL

Plan: Pro Plus

Run ID: 48cd669e-cef0-4c5a-bf65-ea8b2c2dce1d

📥 Commits

Reviewing files that changed from the base of the PR and between 8bc93f6 and 4a48c8c.

📒 Files selected for processing (2)
  • internal/proxy/multitenant.go
  • internal/proxy/multitenant_test.go

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