fix security issues

This commit is contained in:
Gerhard Scheikl
2026-05-09 22:19:25 +02:00
parent c45648832a
commit 3a77bed716
6 changed files with 391 additions and 36 deletions
+65 -6
View File
@@ -25,17 +25,23 @@ import { signGiroCodeUrl } from "../services/invoice/signedUrl";
* - the shop has an IBAN configured.
*/
export const loader = async ({ request }: LoaderFunctionArgs) => {
let sessionToken: { dest?: string } | null = null;
let cors: <T>(res: T) => T = (r) => r;
type AuthSource = "customerAccount" | "checkout";
type SessionTokenLike = { dest?: string; sub?: string };
type CorsFn = (res: Response) => Response;
let sessionToken: SessionTokenLike | null = null;
let cors: CorsFn = (r) => r;
let authSource: AuthSource | null = null;
try {
const auth = await authenticate.public.customerAccount(request);
sessionToken = auth.sessionToken as { dest?: string };
cors = auth.cors;
sessionToken = auth.sessionToken as SessionTokenLike;
cors = auth.cors as CorsFn;
authSource = "customerAccount";
} catch {
try {
const auth = await authenticate.public.checkout(request);
sessionToken = auth.sessionToken as { dest?: string };
cors = auth.cors;
sessionToken = auth.sessionToken as SessionTokenLike;
cors = auth.cors as CorsFn;
authSource = "checkout";
} catch (err) {
throw err;
}
@@ -99,6 +105,44 @@ export const loader = async ({ request }: LoaderFunctionArgs) => {
);
}
// ---- Ownership check ----
// Without this, any authenticated buyer of the shop could enumerate
// arbitrary orderIds and harvest the shop's bank details / amounts.
//
// - customerAccount tokens always carry a customer GID in `sub`. We require
// that the order's customer matches.
// - Checkout (thank-you page) tokens are issued at the end of checkout and
// are short-lived. For logged-in buyers we still bind to the customer
// GID; for guest checkouts (no customer on the order) we fall back to a
// recency window (the order must have been placed in the last hour).
const tokenSub = (sessionToken?.sub ?? "").toString();
const tokenCustomerNumeric = tokenSub.replace(/^gid:\/\/shopify\/[^/]+\//, "").replace(/[^0-9]/g, "");
const orderCustomerNumeric = (orderInfo.customerId ?? "").replace(/^gid:\/\/shopify\/[^/]+\//, "").replace(/[^0-9]/g, "");
let ownershipOk = false;
if (orderCustomerNumeric && tokenCustomerNumeric) {
ownershipOk = orderCustomerNumeric === tokenCustomerNumeric;
} else if (authSource === "checkout" && !orderCustomerNumeric) {
// Guest checkout: no customer to bind against. Accept only if the order
// is fresh (i.e. the buyer has just completed checkout for it).
const placedAtMs = orderInfo.processedAtMs ?? 0;
const RECENT_WINDOW_MS = 60 * 60 * 1000; // 1 hour
ownershipOk = placedAtMs > 0 && Date.now() - placedAtMs <= RECENT_WINDOW_MS;
}
if (!ownershipOk) {
console.warn(
`payment-info: ownership check failed for shop=${shop} order=${orderGid} ` +
`authSource=${authSource} tokenSub=${tokenSub || "-"}`,
);
return cors(
Response.json(
{ showPaymentInstructions: false, error: "forbidden" },
{ status: 403 },
),
);
}
const language = pickLanguage(orderInfo.customerLocale ?? settings.defaultLanguage);
const t = getStrings(language);
@@ -161,6 +205,8 @@ interface OrderInfo {
currency: string;
orderName: string;
customerLocale?: string;
customerId?: string;
processedAtMs?: number;
txCount: number;
manualFlags: Array<{ status?: string; manual?: boolean }>;
}
@@ -176,6 +222,9 @@ async function fetchOrderInfo(
name
currencyCode
customerLocale
processedAt
createdAt
customer { id }
totalPriceSet { shopMoney { amount } }
totalOutstandingSet { shopMoney { amount } }
transactions(first: 20) {
@@ -192,6 +241,9 @@ async function fetchOrderInfo(
name?: string;
currencyCode?: string;
customerLocale?: string | null;
processedAt?: string | null;
createdAt?: string | null;
customer?: { id?: string } | null;
totalPriceSet?: { shopMoney: { amount: string } };
totalOutstandingSet?: { shopMoney: { amount: string } };
transactions?: Array<{ status?: string; manualPaymentGateway?: boolean }>;
@@ -211,6 +263,13 @@ async function fetchOrderInfo(
currency: o.currencyCode ?? "EUR",
orderName: o.name ?? "",
customerLocale: o.customerLocale ?? undefined,
customerId: o.customer?.id ?? undefined,
processedAtMs: (() => {
const raw = o.processedAt ?? o.createdAt ?? null;
if (!raw) return undefined;
const t = Date.parse(raw);
return Number.isFinite(t) ? t : undefined;
})(),
txCount: txs.length,
manualFlags: txs.map((t) => ({ status: t.status, manual: t.manualPaymentGateway })),
};
+31 -2
View File
@@ -31,6 +31,13 @@ interface SettingsFieldErrors {
logo?: string;
}
/**
* Sentinel value used in the SMTP password input. The real password is
* never sent to the client; if the form posts back this exact value the
* action treats it as "unchanged" and keeps whatever is already in the DB.
*/
const SMTP_PASSWORD_SENTINEL = "__unchanged__";
export const loader = async ({ request }: LoaderFunctionArgs) => {
const { session } = await authenticate.admin(request);
const settings = await db.shopSettings.upsert({
@@ -48,7 +55,13 @@ export const loader = async ({ request }: LoaderFunctionArgs) => {
// External HTTPS URL — fine to display directly in the editor.
logoPreviewDataUrl = settings.logoUrl;
}
return { settings, logoPreviewDataUrl };
// Never expose the SMTP password to the browser. We replace it with a
// sentinel and the form action interprets that as "keep existing value".
const safeSettings = {
...settings,
smtpPassword: settings.smtpPassword ? SMTP_PASSWORD_SENTINEL : "",
};
return { settings: safeSettings, logoPreviewDataUrl, smtpPasswordSentinel: SMTP_PASSWORD_SENTINEL };
};
export const action = async ({ request }: ActionFunctionArgs) => {
@@ -134,6 +147,22 @@ export const action = async ({ request }: ActionFunctionArgs) => {
return { ok: false, errors, savedAt: null as string | null };
}
// Resolve SMTP password: the loader sends a sentinel instead of the real
// value. If the form posts that sentinel back unchanged, keep whatever is
// already in the DB; otherwise persist the new value (including the empty
// string, which means "clear the password").
const submittedSmtpPassword = str("smtpPassword");
let nextSmtpPassword: string;
if (submittedSmtpPassword === SMTP_PASSWORD_SENTINEL) {
const current = await db.shopSettings.findUnique({
where: { shopDomain: session.shop },
select: { smtpPassword: true },
});
nextSmtpPassword = current?.smtpPassword ?? "";
} else {
nextSmtpPassword = submittedSmtpPassword;
}
const data = {
companyName: str("companyName"),
legalForm: str("legalForm"),
@@ -167,7 +196,7 @@ export const action = async ({ request }: ActionFunctionArgs) => {
smtpPort: smtpPort ?? 587,
smtpSecure: bool("smtpSecure"),
smtpUser: str("smtpUser"),
smtpPassword: str("smtpPassword"),
smtpPassword: nextSmtpPassword,
smtpFromName: str("smtpFromName"),
smtpFromEmail: str("smtpFromEmail"),
smtpReplyTo: str("smtpReplyTo"),