Merge pull request #889 from tdurieux/fix/registration-email

fix: save verified email addresses during GitHub registration

README.md

@@ -102,6 +102,12 @@ send access alerts through [Resend](https://resend.com/docs/api-reference/emails
 Use a sender address on a domain verified in Resend. `APP_HOSTNAME` sets the
 host used for the connections link in emails.
 
+Registration saves a verified GitHub email address, preferring the primary address.
+OAuth requests `user:email`; GitHub App sign-in uses the read-only email permission
+described in the setup guide. Later sign-ins also fill missing addresses. Saved
+addresses and custom alert addresses are preserved. GitHub privacy placeholder
+addresses are not imported.
+
 Signed-in owners without an email address see an email form in a popup.
 “Remind me later” dismisses it for the current login session; “Never” saves a
 permanent preference on the account. Existing addresses are reused, and owners

docs/github-app-setup.md

@@ -19,7 +19,10 @@ Configure:
 - Repository permissions: **Contents: read-only**, **Metadata: read-only**,
   **Pull requests: read-only**, **Pages: read-only**. Do not grant write permissions.
   Pull requests read also permits [reading PR issue comments](https://docs.github.com/en/rest/issues/comments#list-issue-comments); Issues permission is unnecessary.
-- Keep user access token expiration enabled. Private email permission is unnecessary.
+- User permissions: **Email addresses: read-only** to save a verified address at
+  sign-in, including addresses hidden from the public GitHub profile. If this
+  permission is unavailable, owners can enter an address in the email form.
+- Keep user access token expiration enabled.
 - Leave **Request user authorization (OAuth) during installation** unchecked.
   Anonymous GitHub authorizes the user before opening installation; the setup
   redirect must remain available afterward.

src/core/registration-email.ts

@@ -0,0 +1,41 @@
+import UserModel from "./model/users/users.model";
+import { IUserDocument } from "./model/users/users.types";
+import { normalizeEmail, notificationEmail } from "./owner-notifications";
+
+function usableEmail(value: unknown): string | null {
+  const email = normalizeEmail(value);
+  if (!email) return null;
+  const domain = email.split("@")[1].toLowerCase();
+  return ["users.noreply.github.com", "noreply.github.com"].includes(domain) ? null : email;
+}
+
+/** Fill a missing address after the GitHub identity has been authenticated. */
+export async function saveRegistrationEmail(user: IUserDocument, token: string): Promise<void> {
+  if (notificationEmail(user.emails, user.notificationEmail)) return;
+  // Compare the original fields so a concurrent settings save always wins.
+  const previousEmails = user.toObject().emails;
+  const previousPreference = user.notificationEmail ?? null;
+  try {
+    const response = await fetch("https://api.github.com/user/emails?per_page=100", {
+      headers: { Accept: "application/vnd.github+json", Authorization: `Bearer ${token}`,
+        "X-GitHub-Api-Version": "2022-11-28", "User-Agent": "Anonymous-GitHub" },
+      signal: AbortSignal.timeout(5000),
+    });
+    if (!response.ok) return;
+    const data: unknown = await response.json();
+    if (!Array.isArray(data)) return;
+    const verified = data.filter((entry): entry is { email: string; primary?: boolean } =>
+      !!entry && entry.verified === true && !!usableEmail(entry.email));
+    const email = usableEmail((verified.find(entry => entry.primary === true) || verified[0])?.email);
+    if (!email) return;
+    const emails = [{ email, default: true }];
+    const result = await UserModel.updateOne({ _id: user._id, notificationEmail: previousPreference,
+      $or: [{ emails: previousEmails ?? [] },
+        ...(!previousEmails?.length ? [{ emails: { $exists: false } }] : [])],
+    }, { $set: { emails } });
+    if (result.modifiedCount) user.emails = emails;
+  } catch {
+    // Missing App email permission or an upstream outage must not prevent login.
+    // The existing email form remains available when automatic collection fails.
+  }
+}

src/server/routes/connection.ts

@@ -1,4 +1,5 @@
 import { resetOwnerAccessAlerts } from "../../core/owner-notifications";
+import { saveRegistrationEmail } from "../../core/registration-email";
 import { createClient } from "redis";
 import * as passport from "passport";
 import * as session from "express-session";
@@ -100,12 +101,9 @@ export const verify = async (
           externalIDs: {
             github: profile.id,
           },
-          emails: profile.emails?.map((email) => {
-            return { email: email.value, default: false };
-          }),
+          emails: [],
           photo,
         });
-        if (user.emails?.length) user.emails[0].default = true;
         await user.save();
       }
     }
@@ -121,6 +119,7 @@ export const verify = async (
       return;
     }
     await setCredential(String(user!._id), accessToken);
+    await saveRegistrationEmail(user!, accessToken);
     await resetOwnerAccessAlerts(String(user!._id));
     done(null, { username: user!.username, user });
   } catch (error) {
@@ -208,7 +207,7 @@ router.get(
       returnTo: safeAuthReturnTo(req.query.returnTo, current ? "/connections" : "/dashboard"), expires: Date.now() + 10 * 60000 };
     next();
   },
-  passport.authenticate("github", { scope: ["repo"] }), // Note the scope here
+  passport.authenticate("github", { scope: ["repo", "user:email"] }),
   function (req: express.Request, res: express.Response) {
     res.redirect("/");
   }

src/server/routes/github-app.ts

@@ -1,4 +1,5 @@
 import { notificationEmail, normalizeEmail, resetOwnerAccessAlerts } from "../../core/owner-notifications";
+import { saveRegistrationEmail } from "../../core/registration-email";
 import * as express from "express";
 import { createHmac, randomBytes, randomUUID, timingSafeEqual } from "crypto";
 import config from "../../config";
@@ -94,6 +95,7 @@ router.get("/app/callback", enabled, async (req, res) => {
     }
     if (isDisabledAccount(user.status)) throw appError("not_connected", 403);
     await saveAppGrant(user.id, tokens);
+    await saveRegistrationEmail(user, tokens.access_token);
     await resetOwnerAccessAlerts(user.id);
     await new Promise<void>((resolve, reject) => req.login({ username: user!.username, user }, err => err ? reject(err) : resolve()));
     res.redirect(flow.install ? `/github/app/install?returnTo=${encodeURIComponent(flow.returnTo)}&repository=${encodeURIComponent(flow.repository || "")}` : flow.returnTo);

test/credential-migration.test.js

@@ -194,13 +194,19 @@ describeMongo("credential access (MongoDB)", function () {
   it("OAuth login writes a credential and returns a token-free session user", async () => {
     const passport = require("passport");
     require("../src/server/routes/connection");
-    const result = await new Promise((resolve, reject) => passport._strategy("github")._verify({ githubOAuthContext: { expires: Date.now() + 60000 } }, "oauth-secret", "refresh-secret", {
-      id: "external-test", username: owner.username, emails: [], photos: [],
-    }, (error, user) => error ? reject(error) : resolve(user)));
+    const originalFetch = global.fetch;
+    global.fetch = async () => ({ ok: true, json: async () => [{ email: "private@example.com", primary: true, verified: true }] });
+    let result;
+    try {
+      result = await new Promise((resolve, reject) => passport._strategy("github")._verify({ githubOAuthContext: { expires: Date.now() + 60000 } }, "oauth-secret", "refresh-secret", {
+        id: "external-test", username: owner.username, emails: [], photos: [],
+      }, (error, user) => error ? reject(error) : resolve(user)));
+    } finally { global.fetch = originalFetch; }
     expect(JSON.stringify(result)).not.to.include("oauth-secret");
     expect(JSON.stringify(result)).not.to.include("refresh-secret");
     expect(await getCredentialToken(owner.id)).to.equal("oauth-secret");
     expect((await UserModel.collection.findOne({ _id: owner._id })).accessTokens).to.equal(undefined);
+    expect((await UserModel.findById(owner.id)).emails[0].email).to.equal("private@example.com");
   });
   it("reads hidden legacy resource tokens only during compatibility mode", async () => {
     const resource = await GistModel.collection.insertOne({ gistId: "legacy-gist", owner: owner._id, source: { accessToken: "legacy-resource" } });

test/github-app.test.js

@@ -538,7 +538,34 @@ describeMongo("GitHub App credential and repository integration", function () {
     expect(session.githubAppFlow).to.equal(undefined);
     const replay = await request("/github/app/callback?state=state&code=code");
     expect(replay.status).to.equal(400);
-    expect(calls).to.have.length(2);
+    expect(calls).to.have.length(3);
+  });
+  it("saves a verified private email during App registration", async () => {
+    authenticated = false;
+    session.githubAppFlow = { state: "state", expires: Date.now() + 60000, returnTo: "/dashboard" };
+    mock(url => url.includes("/login/oauth/access_token") ? data() : url.includes("/user/emails?")
+      ? [{ email: "private@example.com", primary: true, verified: true }]
+      : { id: 20, login: "new-owner" });
+    expect((await request("/github/app/callback?state=state&code=code")).location).to.equal("/dashboard");
+    const created = await Users.findOne({ "externalIDs.github": "20" });
+    expect(created.emails[0].email).to.equal("private@example.com");
+    expect(created.emails[0].default).to.equal(true);
+  });
+  it("preserves an email preference saved while App sign-in retrieves emails", async () => {
+    authenticated = false;
+    session.githubAppFlow = { state: "state", expires: Date.now() + 60000, returnTo: "/dashboard" };
+    mock(async url => {
+      if (url.includes("/login/oauth/access_token")) return data();
+      if (url.includes("/user/emails?")) {
+        await Users.updateOne({ _id: owner._id }, { $set: { notificationEmail: "chosen@example.com" } });
+        return [{ email: "github@example.com", primary: true, verified: true }];
+      }
+      return { id: 10, login: "owner" };
+    });
+    expect((await request("/github/app/callback?state=state&code=code")).location).to.equal("/dashboard");
+    const current = await Users.findById(owner.id);
+    expect(current.notificationEmail).to.equal("chosen@example.com");
+    expect(current.emails).to.have.length(0);
   });
   it("signs an existing user in through the App without changing legacy access", async () => {
     authenticated = false;

test/registration-email.test.js

@@ -0,0 +1,135 @@
+/* global fetch, AbortSignal, URL */
+const { expect } = require("chai");
+const express = require("express");
+const session = require("express-session");
+const passport = require("passport");
+require("ts-node/register/transpile-only");
+const Users = require("../src/core/model/users/users.model").default;
+const credentials = require("../src/core/credentials");
+const notifications = require("../src/core/owner-notifications");
+const github = require("../src/core/github-app");
+const { saveRegistrationEmail } = require("../src/core/registration-email");
+const { verify, router } = require("../src/server/routes/connection");
+const { githubAppRouter } = require("../src/server/routes/github-app");
+
+const verified = (email, primary = true) => ({ email, primary, verified: true });
+
+describe("registration email collection", function () {
+  const restores = [];
+  const stub = (object, key, value) => { const old = object[key]; restores.push(() => object[key] = old); object[key] = value; };
+  let response, calls, writes, user, saved, login;
+  beforeEach(() => {
+    response = [verified("primary@example.com")]; calls = []; writes = []; saved = [];
+    user = new Users({ username: "owner", externalIDs: { github: "10" }, emails: [] });
+    stub(global, "fetch", async (url, options) => {
+      calls.push({ url, options });
+      return { ok: true, json: async () => response };
+    });
+    stub(Users, "updateOne", async (filter, update) => { writes.push({ filter, update }); return { modifiedCount: 1 }; });
+    stub(Users, "findOne", async () => user);
+    stub(Users.prototype, "save", async function () { saved.push(this); return this; });
+    stub(credentials, "setCredential", async () => {});
+    stub(notifications, "resetOwnerAccessAlerts", async () => {});
+    stub(github, "saveAppGrant", async () => {});
+    stub(github, "exchangeAppToken", async () => ({ access_token: "fresh-app-token" }));
+    stub(github, "githubRequest", async path => {
+      expect(path).to.equal("/user"); return { id: 10, login: "owner" };
+    });
+    login = () => new Promise((resolve, reject) => verify({ githubOAuthContext: { expires: Date.now() + 60000 } },
+      "fresh-oauth-token", "", { id: "10", username: "owner", emails: [] }, (error, identity) => error ? reject(error) : resolve(identity)));
+  });
+  afterEach(() => { while (restores.length) restores.pop()(); });
+
+  it("saves the verified primary private address instead of an unverified or GitHub placeholder address", async () => {
+    response = [verified("secondary@example.com", false), { email: "unverified@example.com", primary: true, verified: false },
+      verified("123+owner@users.noreply.github.com"), verified(" primary@example.com "), null];
+    await saveRegistrationEmail(user, "fresh-token");
+    expect(user.emails.map(e => ({ email: e.email, default: e.default }))).to.deep.equal([{ email: "primary@example.com", default: true }]);
+    expect(calls[0].url).to.equal("https://api.github.com/user/emails?per_page=100");
+    expect(calls[0].options.headers.Authorization).to.equal("Bearer fresh-token");
+    expect(calls[0].options.signal).to.be.instanceOf(AbortSignal);
+    expect(writes[0].update.$set.emails).to.deep.equal([{ email: "primary@example.com", default: true }]);
+  });
+
+  it("uses a verified secondary address when the primary is a privacy placeholder", async () => {
+    response = [verified("owner@users.noreply.github.com"), verified("secondary@example.com", false)];
+    await saveRegistrationEmail(user, "token");
+    expect(user.emails[0].email).to.equal("secondary@example.com");
+  });
+
+  for (const preference of ["stored-email", "custom-alert"]) it(`preserves ${preference} without requesting private email data`, async () => {
+    if (preference === "stored-email") user.emails = [{ email: "saved@example.com", default: true }];
+    else user.notificationEmail = "custom@example.com";
+    await saveRegistrationEmail(user, "token");
+    expect(calls).to.have.length(0); expect(writes).to.have.length(0);
+  });
+
+  it("does not overwrite a concurrent manual email save", async () => {
+    stub(Users, "updateOne", async (filter) => {
+      expect(filter.notificationEmail).to.equal(null);
+      expect(filter.$or).to.deep.equal([{ emails: [] }, { emails: { $exists: false } }]);
+      return { modifiedCount: 0 };
+    });
+    await saveRegistrationEmail(user, "token");
+    expect(user.emails).to.have.length(0);
+  });
+
+  for (const body of [[], {}, [verified("owner@users.noreply.github.com")], [{ email: "unverified@example.com", verified: false }], [verified("bad\r\nBcc:other@example.com")]]) {
+    it(`leaves the manual prompt available when no usable verified address is returned: ${JSON.stringify(body)}`, async () => {
+      response = body; await saveRegistrationEmail(user, "token");
+      expect(user.emails).to.have.length(0); expect(writes).to.have.length(0);
+    });
+  }
+
+  for (const provider of ["OAuth", "App"]) {
+    async function callback() {
+      if (provider === "OAuth") return (await login()).user;
+      const handler = githubAppRouter.stack.find(l => l.route?.path === "/app/callback").route.stack.at(-1).handle;
+      let identity, location;
+      const req = { query: { code: "code", state: "state" }, session: { githubAppFlow: { state: "state", expires: Date.now() + 60000, returnTo: "/dashboard" }, save: cb => cb() },
+        isAuthenticated: () => false, login: (value, cb) => { identity = value; cb(); } };
+      await handler(req, { redirect: value => { location = value; } });
+      expect(location).to.equal("/dashboard"); return identity.user;
+    }
+    it(`saves an email during new ${provider} registration`, async () => {
+      stub(Users, "findOne", async () => null);
+      const identity = await callback();
+      expect(saved).to.have.length(1); expect(writes).to.have.length(1);
+      expect(identity.emails[0].email).to.equal("primary@example.com");
+    });
+    it(`fills missing email on existing ${provider} sign-in`, async () => {
+      expect((await callback()).emails[0].email).to.equal("primary@example.com");
+      expect(saved).to.have.length(0);
+    });
+    for (const failure of ["permission", "timeout", "database"]) it(`allows ${provider} sign-in when email collection fails due to ${failure}`, async () => {
+      if (failure === "permission") stub(global, "fetch", async () => ({ ok: false, status: 403 }));
+      if (failure === "timeout") stub(global, "fetch", async () => { throw new Error("timeout"); });
+      if (failure === "database") stub(Users, "updateOne", async () => { throw new Error("database unavailable"); });
+      expect((await callback()).id).to.equal(user.id);
+      expect(user.emails).to.have.length(0);
+    });
+  }
+
+  it("rejects an identity mismatch before requesting or saving email", async () => {
+    const error = await new Promise(resolve => verify({ githubOAuthContext: { githubId: "different", expires: Date.now() + 60000 } },
+      "token", "", { id: "10", username: "owner" }, resolve));
+    expect(error.message).to.equal("github_identity_mismatch"); expect(calls).to.have.length(0); expect(writes).to.have.length(0);
+  });
+});
+
+describe("OAuth registration permissions", function () {
+  it("requests private email access on the actual GitHub authorization redirect", async () => {
+    const app = express();
+    app.use(session({ secret: "registration-test", saveUninitialized: false, resave: false }));
+    app.use(passport.initialize()); app.use("/github", router);
+    let server;
+    await new Promise(resolve => { server = app.listen(0, "127.0.0.1", resolve); });
+    try {
+      const response = await fetch(`http://127.0.0.1:${server.address().port}/github/login`, { redirect: "manual" });
+      expect(response.status).to.equal(302);
+      const url = new URL(response.headers.get("location"));
+      expect(url.hostname).to.equal("github.com");
+      expect(url.searchParams.get("scope").split(",")).to.include.members(["repo", "user:email"]);
+    } finally { server.closeAllConnections(); await new Promise(resolve => server.close(resolve)); }
+  });
+});