Skip to content
Open
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
2 changes: 1 addition & 1 deletion packages/drfed/.env.example
Original file line number Diff line number Diff line change
@@ -1,2 +1,2 @@
DRFED_LOGIN_ORIGINS=https://drfed.example.com,http://localhost:3000
DRFED_LOGIN_ORIGIN=https://drfed.example.com

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

The documentation still describes the old interface: packages/drfed/README.md requires DRFED_LOGIN_ORIGINS and shows a command that now fails, while packages/graphql/README.md passes loginOrigins: new Set(...). Please update these examples to match the final configuration and API. The comments in scripts/dev.mts and packages/drfed/src/parser.test.ts also still refer to the removed behavior.

DRFED_ROOT_ORIGIN=http://drfed.localhost:8888
22 changes: 2 additions & 20 deletions packages/drfed/src/index.ts
Original file line number Diff line number Diff line change
Expand Up @@ -41,24 +41,6 @@ import seedData from "./seed.ts";
import { createFetchHandler, warnAboutStrandedInstances } from "./serving.ts";

async function runServer(options: ServerOptions) {
const values = process.env.DRFED_LOGIN_ORIGINS?.split(",").map((value) =>
value.trim(),
);
if (values == null || values.some((value) => value === "")) {
throw new TypeError("DRFED_LOGIN_ORIGINS must contain valid origins.");
}
const loginOrigins = new Set(
values.map((value) => {
const url = new URL(value);
if (url.protocol !== "https:" && url.protocol !== "http:") {
throw new TypeError(
`Unsupported login origin protocol: ${url.protocol}`,
);
}
return url.origin;
}),
);

const { credentials } = options.drizzle;
if (options.drizzle.migrate) await migrate({ credentials });
if (options.seed) await seedData(options.drizzle.db);
Expand All @@ -67,13 +49,13 @@ async function runServer(options: ServerOptions) {
? new PgliteKvStore(credentials.client)
: new PostgresKvStore(credentials.client);
const federation = await createFederation(options.drizzle.db, { kv });
const { emailFrom, mailer, rootOrigin } = options;
const { emailFrom, mailer, rootOrigin, loginOrigin } = options;

const yogaServer = createYogaServer(options.drizzle.db, federation, {
rootOrigin,
emailFrom,
mailer,
loginOrigins,
loginOrigin,
});
await warnAboutStrandedInstances(options.drizzle.db, rootOrigin);
const server = serve({
Expand Down
5 changes: 3 additions & 2 deletions packages/drfed/src/parser.test.ts
Original file line number Diff line number Diff line change
Expand Up @@ -129,16 +129,17 @@ describe("drfed-server", () => {
"--email-from=postmaster@mail.example",
]);
assert.notEqual(accepted.code, 0);
assert.match(accepted.stderr, /DRFED_LOGIN_ORIGINS/u);
assert.match(accepted.stderr, /Missing option .*--login-origin/u);

const rejected = await run([
"--data-path",
dataPath,
"--root-origin=https://drfed.net",
"--login-origin=https://drfed.net",
"--email-from=not-an-address",
]);
assert.notEqual(rejected.code, 0);
assert.doesNotMatch(rejected.stderr, /DRFED_LOGIN_ORIGINS/u);
assert.match(rejected.stderr, /Expected a valid email address/u);
} finally {
await rm(dataPath, { force: true, recursive: true });
}
Expand Down
11 changes: 10 additions & 1 deletion packages/drfed/src/parser.ts
Original file line number Diff line number Diff line change
Expand Up @@ -22,7 +22,7 @@ import { message, optionNames } from "@optique/core/message";
import { map, optional, withDefault } from "@optique/core/modifiers";
import type { InferValue } from "@optique/core/parser";
import { flag, option } from "@optique/core/primitives";
import { email, socketAddress, url } from "@optique/core/valueparser";
import { email, origin, socketAddress, url } from "@optique/core/valueparser";
import { loggingOptions } from "@optique/logtape";
import { path } from "@optique/run/valueparser";
import { LogTapeTransport } from "@upyo/logtape";
Expand Down Expand Up @@ -124,6 +124,14 @@ const emailFromParser = optional(
}),
);

const loginOriginParser = option(

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

This makes --login-origin mandatory and removes support for DRFED_LOGIN_ORIGINS. DRFED_LOGIN_ORIGIN is forwarded only by scripts/dev.mts; setting it for the installed CLI still fails with Missing option --login-origin. Please keep the existing configuration working for the #81 fix, or handle the CLI migration separately and update the documentation.

"--login-origin",
origin({ allowedProtocols: ["http:", "https:"] }),
{
description: message`The frontend origin allowed in email login links.`,
},
);

const serverParser = object("DrFed server", {
address: withDefault(
option("--listen", "-l", socketAddress({ requirePort: true }), {
Expand All @@ -146,6 +154,7 @@ const serverParser = object("DrFed server", {
}),
),
rootOrigin: rootOriginParser,
loginOrigin: loginOriginParser,
emailFrom: emailFromParser,
mailer: smtpParser,
seed: seedParser,
Expand Down
1 change: 1 addition & 0 deletions packages/graphql/src/auth.test.ts
Original file line number Diff line number Diff line change
Expand Up @@ -197,6 +197,7 @@ describe("email authentication", () => {
equal(message.sender.address, "postmaster@mail.example");
},
new URL("https://drfed.example"),
new URL("https://drfed.test"),
"postmaster@mail.example",
);
});
Expand Down
6 changes: 3 additions & 3 deletions packages/graphql/src/auth/expand.ts
Original file line number Diff line number Diff line change
Expand Up @@ -21,14 +21,14 @@ export interface ExpandVerifyUrlParams {
template: Template;
challengeId: `${string}-${string}-${string}-${string}-${string}`;
code: string;
loginOrigins: ReadonlySet<string>;
loginOrigin: string;
}

export default function expandVerifyUrl({
template,
challengeId,
code,
loginOrigins,
loginOrigin,
}: ExpandVerifyUrlParams): string {
assertVariable(template, "challengeId");
assertVariable(template, "code");
Expand All @@ -45,7 +45,7 @@ export default function expandVerifyUrl({
if (url.protocol !== "https:" && url.protocol !== "http:") {
throw invalidVerifyUrl("Verify URL must use HTTP or HTTPS.");
}
if (!loginOrigins.has(url.origin)) {
if (loginOrigin !== url.origin) {
throw invalidVerifyUrl(`Verify URL origin is not allowed: ${url.origin}.`);
}
return url.href;
Expand Down
2 changes: 1 addition & 1 deletion packages/graphql/src/auth/magic-link.ts
Original file line number Diff line number Diff line change
Expand Up @@ -65,7 +65,7 @@ builder.mutationFields((t) => ({
const loginUrl = expandVerifyUrl({
challengeId,
code,
loginOrigins: ctx.loginOrigins,
loginOrigin: ctx.loginOrigin.origin,
template: verifyUrl,
});
const account = await findAccount(email, ctx);
Expand Down
4 changes: 2 additions & 2 deletions packages/graphql/src/builder.ts
Original file line number Diff line number Diff line change
Expand Up @@ -62,9 +62,9 @@ export interface ServerContext {
readonly emailFrom: string;

/**
* Origin list for login.
* Origin for login.
*/
readonly loginOrigins: ReadonlySet<string>;
readonly loginOrigin: URL;

/**
* The root origin of this deployment, which every instance's subdomain is
Expand Down
3 changes: 1 addition & 2 deletions packages/graphql/src/federation.test.ts
Original file line number Diff line number Diff line change
Expand Up @@ -179,11 +179,10 @@ describe("createFederation()", () => {
describe("createYogaServer()", () => {
it("does not mutate the federation instance", async () => {
await withTestHarness(({ db, mailer, federation }) => {
const loginOrigins = new Set(["https://drfed.test"]);
assert.doesNotThrow(() =>
createYogaServer(db, federation, {
mailer,
loginOrigins,
loginOrigin: new URL("https://drfed.test"),
rootOrigin: new URL("https://drfed.test"),
}),
);
Expand Down
4 changes: 2 additions & 2 deletions packages/graphql/src/harness.test.ts
Original file line number Diff line number Diff line change
Expand Up @@ -172,16 +172,16 @@ export async function withTestHarness<T>(
// oxlint-disable-next-line promise/prefer-await-to-callbacks
callback: (harness: TestHarness) => Promise<T> | T,
rootOrigin: URL = new URL("https://drfed.org"),
loginOrigin: URL = new URL("https://drfed.test"),
emailFrom?: string,
): Promise<Awaited<T>> {
return await withTemporaryDatabase(async (db) => {
const mailer = new MockTransport();
const federation = await createFederation(db, { kv: new MemoryKvStore() });
const loginOrigins = new Set(["https://drfed.test"]);
const yoga = createYogaServer(db, federation, {
mailer,
loginOrigins,
rootOrigin,
loginOrigin,
emailFrom,
});
const fetch: TestFetch = yoga.fetch.bind(yoga);
Expand Down
6 changes: 3 additions & 3 deletions packages/graphql/src/index.ts
Original file line number Diff line number Diff line change
Expand Up @@ -46,9 +46,9 @@ export interface YogaServerOptions {
emailFrom?: string | undefined;

/**
* Origin list for login.
* Origin for login.
*/
loginOrigins: ReadonlySet<string>;
loginOrigin: URL;

/**
* The root origin of this deployment. Every instance is served from a
Expand Down Expand Up @@ -116,7 +116,7 @@ const fillOptions = (
// at drfed.org would fail the SPF and DMARC checks of every deployment but
// the project's own, and the login mail would be rejected or junked.
emailFrom: opt.emailFrom ?? `noreply@${canonicalHostname(opt.rootOrigin)}`,
loginOrigins: opt.loginOrigins,
loginOrigin: opt.loginOrigin,

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

The CLI normalizes the trailing dot, but direct createYogaServer() callers bypass that parser. I reproduced this through loginByEmail: loginOrigin: new URL("https://app.example.") rejects a verify URL on https://app.example. Please normalize the configured origin at the GraphQL API boundary too.

rootOrigin: opt.rootOrigin,
});

Expand Down
3 changes: 2 additions & 1 deletion scripts/dev.mts
Original file line number Diff line number Diff line change
Expand Up @@ -49,6 +49,7 @@ try {
// DNS or /etc/hosts setup, which is what makes per-instance subdomains usable
// in development.
const defaultRootOrigin = "http://drfed.localhost:8888";
const defaultLoginOrigin = "http://drfed.localhost:3000";

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

When DRFED_LOGIN_ORIGIN is unset, this allows only http://drfed.localhost:3000, but the frontend dev server advertises http://localhost:3000/. Sign-in builds its verify URL from location.origin, so opening the advertised URL causes login to fail with an unauthorized origin. Please default to http://localhost:3000, or configure the frontend to serve and advertise drfed.localhost:3000 too.

const isWindows = process.platform === "win32";
const pnpm = isWindows ? "pnpm.cmd" : "pnpm";

Expand Down Expand Up @@ -317,13 +318,13 @@ try {

const serverArgs: string[] = [
"--watch",
"--env-file=.env",
"bin/drfed-server.mjs",
"--pglite-data-path",
"../../.pgdata",
"--listen=0.0.0.0:8888",
"--log-format=color",
`--root-origin=${process.env.DRFED_ROOT_ORIGIN ?? defaultRootOrigin}`,
`--login-origin=${process.env.DRFED_LOGIN_ORIGIN ?? defaultLoginOrigin}`,
];

const logLevel = process.env.usage_log_level;
Expand Down
Loading