diff --git a/README.md b/README.md index 4484c70..c070301 100644 --- a/README.md +++ b/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 diff --git a/docs/github-app-setup.md b/docs/github-app-setup.md index 5000a73..daf9599 100644 --- a/docs/github-app-setup.md +++ b/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. diff --git a/src/core/registration-email.ts b/src/core/registration-email.ts new file mode 100644 index 0000000..68f038e --- /dev/null +++ b/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 { + 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. + } +} diff --git a/src/server/routes/connection.ts b/src/server/routes/connection.ts index 1e1fe00..54f7901 100644 --- a/src/server/routes/connection.ts +++ b/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("/"); } diff --git a/src/server/routes/github-app.ts b/src/server/routes/github-app.ts index fb979c2..347429c 100644 --- a/src/server/routes/github-app.ts +++ b/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((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); diff --git a/test/credential-migration.test.js b/test/credential-migration.test.js index 08a4848..9947af6 100644 --- a/test/credential-migration.test.js +++ b/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" } }); diff --git a/test/github-app.test.js b/test/github-app.test.js index a1fb00d..635ecaa 100644 --- a/test/github-app.test.js +++ b/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; diff --git a/test/registration-email.test.js b/test/registration-email.test.js new file mode 100644 index 0000000..4cc6432 --- /dev/null +++ b/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)); } + }); +});