Skip to content

Bare-id OAuth state for shared playgrounds + reconciliation tests - #35

Merged
tpaulshippy merged 1 commit into
mainfrom
fix/callback-bare-id-state
Oct 20, 2026
Merged

tpaulshippy merged 1 commit into
mainfrom
fix/callback-bare-id-state

Conversation

@tpaulshippy

@tpaulshippy tpaulshippy commented Oct 20, 2026 •

Copy link
Copy Markdown
Owner

Members log in from the bare-id join cookie, which the callback gate rejected outright (members could never finish login on a canonical-hosted playground). Accepting it naively would let the canonical flow treat a foreign object as the caller's pre-login device (export + clear), so bare-id states on non-shared objects are still refused before any adoption runs.

New tests/sharing-canonical.test.ts (8 tests) pins the sharing x canonical reconciliation: invite id shape, join routing, member-token git ops, member/non-member callback, migration skip, bare-id anonymity, bare-id callback rejection.


Summary by cubic

Members logging in from a shared playground's bare-id join cookie can now finish GitHub OAuth — the callback previously rejected bare-id states outright. Bare-id states on non-shared objects are still refused before any adoption runs, so the canonical flow never treats a foreign object as the caller's pre-login device.

Tests

  • Adds tests/sharing-canonical.test.ts pinning sharing x canonical reconciliation: invite id shape, join routing, member-token git ops, member/non-member callbacks, migration skip, and bare-id anonymity and rejection.

Written for commit f86f03e. Summary will update on new commits.

View guided diff Turn on auto-fix

Members log in from the bare-id join cookie, which the callback gate
rejected outright. Accepting it naively would let the canonical flow
treat a foreign object as the caller's pre-login device (export +
clear), so bare-id states on non-shared objects are still refused
before any adoption runs.

Tests: new tests/sharing-canonical.test.ts pins the sharing x
canonical reconciliation (invite id shape, join routing, member-token
git ops, member/non-member callback, migration skip, bare-id anonymity).
@tpaulshippy
tpaulshippy merged commit 551b804 into main Oct 20, 2026
3 checks passed
@tpaulshippy

Copy link
Copy Markdown
Owner Author

@cubic-dev-ai please post-merge review this PR. It was merged before your review completed (your check showed skipping). The change touches OAuth callback auth and session routing in src/index.ts, plus a reconciliation of shared playgrounds with canonical sessions — security-sensitive, so a retroactive review would be valuable. Thanks!

@cubic-dev-ai

cubic-dev-ai Bot commented Oct 20, 2026

Copy link
Copy Markdown

@cubic-dev-ai please post-merge review this PR. It was merged before your review completed (your check showed skipping). The change touches OAuth callback auth and session routing in src/index.ts, plus a reconciliation of shared playgrounds with canonical sessions — security-sensitive, so a retroactive review would be valuable. Thanks!

@tpaulshippy I have started the AI code review. It will take a few minutes to complete.

@cubic-dev-ai cubic-dev-ai 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.

1 issue found across 2 files

Prompt for AI agents (unresolved issues)

Check if these issues are valid — if so, understand the root cause of each and fix them. When an issue isn't valid or won't be fixed in this PR, reply in its thread with the reason and then resolve the thread. If appropriate, use sub-agents to investigate and fix each issue separately.


<file name="src/index.ts">

<violation number="1" location="src/index.ts:1128">
P1: Bare canonical IDs are public, so this state check no longer prevents OAuth login CSRF. An attacker can authorize their own GitHub account and navigate a member's browser to this callback; the member cookie is sent and `setMemberToken` links the attacker's token to that slot. Generate and validate a one-time nonce bound to the member/browser instead of using the bare ID as OAuth state.</violation>
</file>

Reply with feedback, questions, or to request a fix.

View guided diff | Re-trigger cubic

Comment thread src/index.ts Resolved
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