Skip to content

fix(auth): derive the Google hd hint from an address-only allow list - #213

Open
R0drig0Diaz wants to merge 1 commit into
trycompai:mainfrom
R0drig0Diaz:fix/hd-hint-from-address-allowlist
Open

R0drig0Diaz wants to merge 1 commit into
trycompai:mainfrom
R0drig0Diaz:fix/hd-hint-from-address-allowlist

Conversation

@R0drig0Diaz

@R0drig0Diaz R0drig0Diaz commented Sep 10, 2026

Copy link
Copy Markdown

Behaviour change, so flagging it for discussion rather than assuming it is wanted.

The asymmetry

packages/auth/src/workspace.ts:

export function primaryWorkspaceDomain(): string | undefined {
	return allowList().domains[0];
}

allowList() sorts each entry into domains or addresses on whether it contains @. So an address-only list leaves domains empty, primaryWorkspaceDomain() returns undefined, and auth.ts:48 never sets google.hd.

The result is that two installs naming the same single workspace behave differently:

ALLOWED_SIGN_IN hd sent Account chooser
acme.com yes narrowed to the workspace
rep@acme.com no every Google account the user has

.env.example and docs/environment.md both present the bare address as the form for a solo self-hoster, where gmail.com as a domain would be an open door. That user gets the worse chooser precisely because they chose the tighter allow list.

The change

Derive the domain from the addresses when no bare domain is configured, and only when they all share one. hd narrows the chooser to a single domain, so sending it for one of several would hide the others rather than help.

What this is not

hd on the authorization request is an account-chooser hint, not a control. The request parameter is tamperable, and the verifiable claim is hd on the returned ID token. This PR does not treat it as a boundary and does not change any boundary:

  • isWorkspaceEmail() is untouched, so who may sign in is exactly as before. The user.create.before hook in auth.ts remains the enforcement point.
  • workspaceDomains() is untouched deliberately. It is read by apps/api/src/mailbox/mailbox-match.service.ts to decide which side of a thread is internal. Deriving a domain there would reclassify a colleague as a lead, which is the drift docs/environment.md warns about when it says one list is read by both the sign-in guard and the sync.

So the gain is one fewer wrong-account round trip, not added security.

Tests

New packages/auth/test/workspace.spec.ts, 6 cases, covering the derived hint, the configured-domain precedence, the multi-domain abstention, the empty list, and an explicit assertion that the allow list did not widen. All pass.

Open question for maintainers

When addresses span several domains this abstains. The alternative is to send the first, which would make the chooser useful for one user and broken for the rest. Abstaining seemed the safer default, but it is a judgement call and worth confirming.

🤖 Generated with Claude Code


Summary by cubic

Derives the Google hd hint from an address-only allow list so a solo self-hoster on me@acme.com gets the same narrowed account chooser as one on acme.com. Previously, an allow list with only addresses produced no hint because primaryWorkspaceDomain() looked only for bare domains.

  • The hint is sent only when all allowed addresses share one domain; multi-domain lists abstain to avoid hiding valid accounts.
  • Sign-in enforcement and thread-internal classification are unchanged; isWorkspaceEmail() and workspaceDomains() are untouched.

Written for commit 2d43c99. Summary will update on new commits.

Review in cubic

`primaryWorkspaceDomain()` reads `allowList().domains[0]`, so it returns
undefined whenever ALLOWED_SIGN_IN holds only addresses. `auth.ts` then omits
`google.hd`, and a solo self-hoster on `me@acme.com` gets the full account
chooser while one on `acme.com` does not, despite both naming one workspace.

Derive the domain from the addresses when no bare domain is configured, and
only when every address shares one: `hd` narrows the chooser to a single
domain, so sending it for one of several would hide the rest.

`isWorkspaceEmail()` is untouched, so who may sign in does not change.
`workspaceDomains()` is untouched deliberately: it drives the sync decision
about which side of a thread is internal, and widening it would file a
colleague as a lead.

Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
@vercel

vercel Bot commented Sep 10, 2026

Copy link
Copy Markdown

@R0drig0Diaz is attempting to deploy a commit to the Comp AI - PoC Team on Vercel.

A member of the Team first needs to authorize it.

@cubic-dev-ai cubic-dev-ai Bot left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

3 issues 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. If appropriate, use sub-agents to investigate and fix each issue separately.


<file name="packages/auth/src/workspace.ts">

<violation number="1" location="packages/auth/src/workspace.ts:40">
P2: AGENTS.md forbids all code comments ("Never add code comments. Not to new code, not to code you edit."), and this change adds a four-line comment block in production code. Remove the comment; the branch logic is self-explanatory. The added comment in packages/auth/test/workspace.spec.ts (`// hd narrows the chooser to one domain...`) violates the same rule.</violation>

<violation number="2" location="packages/auth/src/workspace.ts:45">
P2: When an address entry has no host, `.filter(Boolean)` drops it and a valid sibling can produce `hd` even though not all entries share a domain. Preserve the empty host or reject derivation whenever any address lacks a host.</violation>

<violation number="3" location="packages/auth/src/workspace.ts:48">
P2: When an address-only allow-list rejects `other@acme.com`, this derived value makes the guard say `Sign in with your @acme.com account` even though that address is refused. Keep the Google `hd` derivation separate from the rejection-message domain, or use the generic allow-list error for address-only lists.</violation>
</file>

Tip: cubic used a learning from your PR history. Let your coding agent read cubic learnings directly with the cubic MCP.

Fix all with cubic | Re-trigger cubic

// `acme.com`. Only when every allowed address shares a domain: `hd` narrows
// the chooser to one, so sending it for one of several would hide the rest.
const hosts = new Set(
addresses.map((address) => address.split("@")[1]).filter(Boolean),

@cubic-dev-ai cubic-dev-ai Bot Sep 10, 2026

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

P2: When an address entry has no host, .filter(Boolean) drops it and a valid sibling can produce hd even though not all entries share a domain. Preserve the empty host or reject derivation whenever any address lacks a host.

Prompt for AI agents
Check if this issue is valid — if so, understand the root cause and fix it. At packages/auth/src/workspace.ts, line 45:

<comment>When an address entry has no host, `.filter(Boolean)` drops it and a valid sibling can produce `hd` even though not all entries share a domain. Preserve the empty host or reject derivation whenever any address lacks a host.</comment>

<file context>
@@ -33,7 +33,19 @@ export function workspaceDomains(): readonly string[] {
+	// `acme.com`. Only when every allowed address shares a domain: `hd` narrows
+	// the chooser to one, so sending it for one of several would hide the rest.
+	const hosts = new Set(
+		addresses.map((address) => address.split("@")[1]).filter(Boolean),
+	);
+
</file context>
Suggested change
addresses.map((address) => address.split("@")[1]).filter(Boolean),
addresses.map((address) => address.split("@")[1]),
Fix with cubic

addresses.map((address) => address.split("@")[1]).filter(Boolean),
);

return hosts.size === 1 ? [...hosts][0] : undefined;

@cubic-dev-ai cubic-dev-ai Bot Sep 10, 2026

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

P2: When an address-only allow-list rejects other@acme.com, this derived value makes the guard say Sign in with your @acme.com account even though that address is refused. Keep the Google hd derivation separate from the rejection-message domain, or use the generic allow-list error for address-only lists.

Prompt for AI agents
Check if this issue is valid — if so, understand the root cause and fix it. At packages/auth/src/workspace.ts, line 48:

<comment>When an address-only allow-list rejects `other@acme.com`, this derived value makes the guard say `Sign in with your @acme.com account` even though that address is refused. Keep the Google `hd` derivation separate from the rejection-message domain, or use the generic allow-list error for address-only lists.</comment>

<file context>
@@ -33,7 +33,19 @@ export function workspaceDomains(): readonly string[] {
+		addresses.map((address) => address.split("@")[1]).filter(Boolean),
+	);
+
+	return hosts.size === 1 ? [...hosts][0] : undefined;
 }
 
</file context>
Fix with cubic


if (domains[0]) return domains[0];

// An address-only allow list still names a workspace domain, and a solo

@cubic-dev-ai cubic-dev-ai Bot Sep 10, 2026

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

P2: AGENTS.md forbids all code comments ("Never add code comments. Not to new code, not to code you edit."), and this change adds a four-line comment block in production code. Remove the comment; the branch logic is self-explanatory. The added comment in packages/auth/test/workspace.spec.ts (// hd narrows the chooser to one domain...) violates the same rule.

Prompt for AI agents
Check if this issue is valid — if so, understand the root cause and fix it. At packages/auth/src/workspace.ts, line 40:

<comment>AGENTS.md forbids all code comments ("Never add code comments. Not to new code, not to code you edit."), and this change adds a four-line comment block in production code. Remove the comment; the branch logic is self-explanatory. The added comment in packages/auth/test/workspace.spec.ts (`// hd narrows the chooser to one domain...`) violates the same rule.</comment>

<file context>
@@ -33,7 +33,19 @@ export function workspaceDomains(): readonly string[] {
+
+	if (domains[0]) return domains[0];
+
+	// An address-only allow list still names a workspace domain, and a solo
+	// self-hoster on `me@acme.com` wants the same account chooser as one on
+	// `acme.com`. Only when every allowed address shares a domain: `hd` narrows
</file context>
Fix with cubic

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