fix(auth): derive the Google hd hint from an address-only allow list - #213
R0drig0Diaz wants to merge 1 commit into
Conversation
`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>
|
@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. |
There was a problem hiding this comment.
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), |
There was a problem hiding this comment.
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>
| addresses.map((address) => address.split("@")[1]).filter(Boolean), | |
| addresses.map((address) => address.split("@")[1]), |
| addresses.map((address) => address.split("@")[1]).filter(Boolean), | ||
| ); | ||
|
|
||
| return hosts.size === 1 ? [...hosts][0] : undefined; |
There was a problem hiding this comment.
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>
|
|
||
| if (domains[0]) return domains[0]; | ||
|
|
||
| // An address-only allow list still names a workspace domain, and a solo |
There was a problem hiding this comment.
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>
Behaviour change, so flagging it for discussion rather than assuming it is wanted.
The asymmetry
packages/auth/src/workspace.ts:allowList()sorts each entry intodomainsoraddresseson whether it contains@. So an address-only list leavesdomainsempty,primaryWorkspaceDomain()returns undefined, andauth.ts:48never setsgoogle.hd.The result is that two installs naming the same single workspace behave differently:
ALLOWED_SIGN_INhdsentacme.comrep@acme.com.env.exampleanddocs/environment.mdboth present the bare address as the form for a solo self-hoster, wheregmail.comas 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.
hdnarrows the chooser to a single domain, so sending it for one of several would hide the others rather than help.What this is not
hdon the authorization request is an account-chooser hint, not a control. The request parameter is tamperable, and the verifiable claim ishdon 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. Theuser.create.beforehook inauth.tsremains the enforcement point.workspaceDomains()is untouched deliberately. It is read byapps/api/src/mailbox/mailbox-match.service.tsto decide which side of a thread is internal. Deriving a domain there would reclassify a colleague as a lead, which is the driftdocs/environment.mdwarns 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
hdhint from an address-only allow list so a solo self-hoster onme@acme.comgets the same narrowed account chooser as one onacme.com. Previously, an allow list with only addresses produced no hint becauseprimaryWorkspaceDomain()looked only for bare domains.isWorkspaceEmail()andworkspaceDomains()are untouched.Written for commit 2d43c99. Summary will update on new commits.