From e4c2ccc4a8951c364e05c9b2dc67767806847608 Mon Sep 17 00:00:00 2001 From: Fadhlan Date: Wed, 29 Jul 2026 14:53:59 +0700 Subject: [PATCH 1/3] fix(settings): restrict custom provider to local development --- esbuild.config.mjs | 1 + src/settings/settings.test.ts | 50 ++++++++++++++++++++++++++++++++--- src/settings/settings.ts | 30 +++++++++++++++++---- src/settings/tab.ts | 19 ++++++++++++- src/storage/storage.test.ts | 11 ++++++++ src/storage/storage.ts | 15 ++++++----- 6 files changed, 111 insertions(+), 15 deletions(-) diff --git a/esbuild.config.mjs b/esbuild.config.mjs index bdfc1a8..68cd9bd 100644 --- a/esbuild.config.mjs +++ b/esbuild.config.mjs @@ -10,6 +10,7 @@ const context = await esbuild.context({ format: "cjs", target: "es2020", platform: "browser", + define: { "process.env.NODE_ENV": JSON.stringify(production ? "production" : "development") }, sourcemap: production ? false : "inline", minify: production, outfile: "main.js", diff --git a/src/settings/settings.test.ts b/src/settings/settings.test.ts index a7e496c..d9e4694 100644 --- a/src/settings/settings.test.ts +++ b/src/settings/settings.test.ts @@ -7,6 +7,7 @@ import { type GeodeSettings, hasConnectionConfig, normalizeSettings, + providerOptions, regionFor, settingsEqual, } from "./settings.ts"; @@ -48,9 +49,9 @@ const normalizeCases: { name: string; input: unknown; want: GeodeSettings }[] = want: { ...DEFAULT_SETTINGS, bucket: "my-bucket" }, }, { - name: "provider s3 coerced to r2", - input: { provider: "s3" }, - want: DEFAULT_SETTINGS, + name: "provider s3 preserved", + input: { provider: "s3", region: "eu-west-2" }, + want: { ...DEFAULT_SETTINGS, provider: "s3", region: "eu-west-2" }, }, { name: "provider 42 coerced to r2", @@ -101,6 +102,11 @@ const endpointCases: { name: string; input: GeodeSettings; want: string }[] = [ input: { ...DEFAULT_SETTINGS, accountId: "abc123" }, want: "https://abc123.r2.cloudflarestorage.com", }, + { + name: "amazon s3", + input: { ...DEFAULT_SETTINGS, provider: "s3", region: "eu-west-2" }, + want: "https://s3.eu-west-2.amazonaws.com", + }, { name: "custom", input: { ...DEFAULT_SETTINGS, provider: "custom", endpoint: "https://s3.example.com" }, @@ -133,6 +139,21 @@ for (const { name, input, want } of regionCases) { }); } +test("providerOptions: production excludes the custom provider", () => { + assert.deepStrictEqual(providerOptions(false), { + r2: "Cloudflare R2", + s3: "Amazon S3", + }); +}); + +test("providerOptions: local development includes the custom provider", () => { + assert.deepStrictEqual(providerOptions(true), { + r2: "Cloudflare R2", + s3: "Amazon S3", + custom: "Custom", + }); +}); + const settingsEqualCases: { name: string; a: GeodeSettings; b: GeodeSettings; want: boolean }[] = [ { name: "identical values are equal", @@ -188,6 +209,29 @@ const hasConnectionConfigCases: { name: string; input: GeodeSettings; want: bool input: { ...DEFAULT_SETTINGS, bucket: "b", accessKeyId: "a", secretId: "s" }, want: false, }, + { + name: "s3 missing region is incomplete", + input: { + ...DEFAULT_SETTINGS, + provider: "s3", + bucket: "b", + accessKeyId: "a", + secretId: "s", + }, + want: false, + }, + { + name: "s3 with all fields is complete", + input: { + ...DEFAULT_SETTINGS, + provider: "s3", + region: "us-east-1", + bucket: "b", + accessKeyId: "a", + secretId: "s", + }, + want: true, + }, { name: "custom missing region is incomplete", input: { diff --git a/src/settings/settings.ts b/src/settings/settings.ts index 5309083..9f52148 100644 --- a/src/settings/settings.ts +++ b/src/settings/settings.ts @@ -10,10 +10,13 @@ export const DEFAULT_SETTINGS: GeodeSettings = { secretId: "", }; +// Provider identifies a supported S3 compatible storage configuration. +export type Provider = "r2" | "s3" | "custom"; + // GeodeSettings is the persisted shape of a Geode plugin's user configuration. export type GeodeSettings = { version: number; - provider: "r2" | "custom"; + provider: Provider; accountId: string; endpoint: string; region: string; @@ -47,6 +50,9 @@ export function endpointFor(settings: GeodeSettings): string { if (settings.provider === "r2") { return `https://${settings.accountId}.r2.cloudflarestorage.com`; } + if (settings.provider === "s3") { + return `https://s3.${settings.region}.amazonaws.com`; + } return settings.endpoint; } @@ -59,6 +65,9 @@ export function hasConnectionConfig(settings: GeodeSettings): boolean { if (settings.provider === "r2") { return settings.accountId !== ""; } + if (settings.provider === "s3") { + return settings.region !== ""; + } return settings.endpoint !== "" && settings.region !== ""; } @@ -83,14 +92,25 @@ export function normalizeSettings(raw: unknown): GeodeSettings { }; } -// providerOr returns "custom" if v is "custom", otherwise "r2". -export function providerOr(v: unknown): "r2" | "custom" { - if (v === "custom") { - return "custom"; +// providerOr returns a known provider, defaulting unknown values to "r2". +export function providerOr(v: unknown): Provider { + if (v === "s3" || v === "custom") { + return v; } return "r2"; } +// providerOptions returns user-facing providers, including Custom only for local development. +export function providerOptions( + localDev: boolean, +): Record | Record<"r2" | "s3", string> { + if (localDev) { + return { r2: "Cloudflare R2", s3: "Amazon S3", custom: "Custom" }; + } + + return { r2: "Cloudflare R2", s3: "Amazon S3" }; +} + // regionFor returns the signing region to use for the given settings. R2 always signs with // "auto" regardless of what a user might type, so custom is the only provider that needs one. export function regionFor(settings: GeodeSettings): string { diff --git a/src/settings/tab.ts b/src/settings/tab.ts index 3aa14dc..965f2eb 100644 --- a/src/settings/tab.ts +++ b/src/settings/tab.ts @@ -13,6 +13,7 @@ import { draftForDisplay, type GeodeSettings, hasConnectionConfig, + providerOptions, providerOr, settingsEqual, } from "./settings"; @@ -198,6 +199,22 @@ function renderProviderFields(tab: GeodeSettingTab, containerEl: HTMLElement): v return; } + if (tab.draft.provider === "s3") { + new Setting(containerEl) + .setName("Region") + .setDesc("The AWS region your bucket lives in.") + .addText((text) => + text + .setPlaceholder("us-east-1") + .setValue(tab.draft.region) + .onChange((value) => { + tab.draft.region = value; + onFieldChanged(tab); + }), + ); + return; + } + new Setting(containerEl) .setName("Endpoint") .setDesc("The S3 compatible endpoint URL for your storage.") @@ -260,7 +277,7 @@ function renderStorageSection(tab: GeodeSettingTab, containerEl: HTMLElement): v .setDesc("Where your vault is synced to.") .addDropdown((dropdown) => dropdown - .addOptions({ r2: "Cloudflare R2", custom: "Custom" }) + .addOptions(providerOptions(process.env.NODE_ENV !== "production")) .setValue(tab.draft.provider) .onChange((value) => { tab.draft.provider = providerOr(value); diff --git a/src/storage/storage.test.ts b/src/storage/storage.test.ts index 870dc12..310ed44 100644 --- a/src/storage/storage.test.ts +++ b/src/storage/storage.test.ts @@ -70,6 +70,17 @@ const missingFieldCases: { secretAccessKey: "shh", want: "Fill in account ID first", }, + { + name: "missing region for Amazon S3", + settings: { + ...DEFAULT_SETTINGS, + provider: "s3", + bucket: "my-vault", + accessKeyId: "AKIA123", + }, + secretAccessKey: "shh", + want: "Fill in region first", + }, { name: "missing endpoint for custom", settings: { diff --git a/src/storage/storage.ts b/src/storage/storage.ts index 709a2ba..94b6188 100644 --- a/src/storage/storage.ts +++ b/src/storage/storage.ts @@ -221,8 +221,8 @@ function conditionHeaders(condition: PutCondition | undefined): Record Date: Wed, 5 Aug 2026 14:49:50 +0700 Subject: [PATCH 2/3] fix(settings): validate the Amazon S3 region before building an endpoint endpointFor interpolates the persisted region straight into the URL authority for the s3 provider, so a region containing authority delimiters (x@attacker.example:443#) redirects signed connection and sync requests, vault data included, to an unintended host. Validate the region as an AWS region identifier: endpointFor returns "" for anything else, hasConnectionConfig treats it as incomplete, and testConnection reports it as a missing region with an example. --- src/settings/settings.test.ts | 49 +++++++++++++++++++++++++++++++++++ src/settings/settings.ts | 15 ++++++++++- src/storage/storage.ts | 7 ++++- 3 files changed, 69 insertions(+), 2 deletions(-) diff --git a/src/settings/settings.test.ts b/src/settings/settings.test.ts index d9e4694..bf32cbe 100644 --- a/src/settings/settings.test.ts +++ b/src/settings/settings.test.ts @@ -6,6 +6,7 @@ import { endpointFor, type GeodeSettings, hasConnectionConfig, + isAwsRegion, normalizeSettings, providerOptions, regionFor, @@ -112,6 +113,21 @@ const endpointCases: { name: string; input: GeodeSettings; want: string }[] = [ input: { ...DEFAULT_SETTINGS, provider: "custom", endpoint: "https://s3.example.com" }, want: "https://s3.example.com", }, + { + name: "amazon s3 with a region carrying URL authority delimiters yields no endpoint", + input: { ...DEFAULT_SETTINGS, provider: "s3", region: "x@attacker.example:443#" }, + want: "", + }, + { + name: "amazon s3 with a region carrying a path separator yields no endpoint", + input: { ...DEFAULT_SETTINGS, provider: "s3", region: "us-east-1/../evil" }, + want: "", + }, + { + name: "amazon s3 with an empty region yields no endpoint", + input: { ...DEFAULT_SETTINGS, provider: "s3", region: "" }, + want: "", + }, ]; for (const { name, input, want } of endpointCases) { @@ -120,6 +136,27 @@ for (const { name, input, want } of endpointCases) { }); } +const awsRegionCases: { region: string; want: boolean }[] = [ + { region: "us-east-1", want: true }, + { region: "eu-west-2", want: true }, + { region: "ap-southeast-1", want: true }, + { region: "us-gov-west-1", want: true }, + { region: "", want: false }, + { region: "US-EAST-1", want: false }, + { region: "us-east", want: false }, + { region: "us-east-1 ", want: false }, + { region: "x@attacker.example:443#", want: false }, + { region: "us-east-1@attacker.example", want: false }, + { region: "us-east-1/../evil", want: false }, + { region: "us-east-1\nx", want: false }, +]; + +for (const { region, want } of awsRegionCases) { + test(`isAwsRegion: ${JSON.stringify(region)} is ${want}`, () => { + assert.strictEqual(isAwsRegion(region), want); + }); +} + const regionCases: { name: string; input: GeodeSettings; want: string }[] = [ { name: "r2 always signs as auto", @@ -232,6 +269,18 @@ const hasConnectionConfigCases: { name: string; input: GeodeSettings; want: bool }, want: true, }, + { + name: "s3 with a region that is not an AWS region identifier is incomplete", + input: { + ...DEFAULT_SETTINGS, + provider: "s3", + region: "x@attacker.example:443#", + bucket: "b", + accessKeyId: "a", + secretId: "s", + }, + want: false, + }, { name: "custom missing region is incomplete", input: { diff --git a/src/settings/settings.ts b/src/settings/settings.ts index 9f52148..9324d19 100644 --- a/src/settings/settings.ts +++ b/src/settings/settings.ts @@ -46,11 +46,17 @@ export function draftForDisplay( } // endpointFor returns the storage endpoint URL to use for the given settings. +// The Amazon S3 endpoint interpolates the region into the URL authority, so a region carrying +// authority delimiters (`x@attacker.example:443#`) would silently redirect signed vault requests +// to another host. An unrecognised region yields "" rather than a host we never meant to talk to. export function endpointFor(settings: GeodeSettings): string { if (settings.provider === "r2") { return `https://${settings.accountId}.r2.cloudflarestorage.com`; } if (settings.provider === "s3") { + if (!isAwsRegion(settings.region)) { + return ""; + } return `https://s3.${settings.region}.amazonaws.com`; } @@ -66,11 +72,18 @@ export function hasConnectionConfig(settings: GeodeSettings): boolean { return settings.accountId !== ""; } if (settings.provider === "s3") { - return settings.region !== ""; + return isAwsRegion(settings.region); } return settings.endpoint !== "" && settings.region !== ""; } +// isAwsRegion reports whether region looks like an AWS region identifier ("us-east-1", +// "eu-west-2", "us-gov-west-1"). Only the restricted alphabet matters for safety: it admits no +// character that can terminate or redirect a URL authority. +export function isAwsRegion(region: string): boolean { + return /^[a-z]{2}(-[a-z]+){1,2}-\d{1,2}$/.test(region); +} + // normalizeSettings returns a complete GeodeSettings from whatever loadData produced, // filling gaps with defaults. export function normalizeSettings(raw: unknown): GeodeSettings { diff --git a/src/storage/storage.ts b/src/storage/storage.ts index 94b6188..0014f7e 100644 --- a/src/storage/storage.ts +++ b/src/storage/storage.ts @@ -1,5 +1,5 @@ import { AwsClient } from "aws4fetch"; -import { endpointFor, type GeodeSettings, regionFor } from "../settings/settings.ts"; +import { endpointFor, type GeodeSettings, isAwsRegion, regionFor } from "../settings/settings.ts"; import { encodeComponent, encodeKey } from "./encode.ts"; import { messageFor, statusForHttp } from "./errors.ts"; import { parseListObjectsXml } from "./xml.ts"; @@ -249,6 +249,11 @@ function missingFieldFor(settings: GeodeSettings, secretAccessKey: string): stri if (settings.region === "") { return "region"; } + // Amazon S3 builds its endpoint host from the region, so a region that isn't a real region + // identifier has no endpoint to sign against and is reported the same as a missing one. + if (settings.provider === "s3" && !isAwsRegion(settings.region)) { + return "region (for example us-east-1)"; + } return ""; } From dde4cfb16d725e439c2c1daef3b73f2c4352c759 Mon Sep 17 00:00:00 2001 From: revett <2796074+revett@users.noreply.github.com> Date: Mon, 10 Aug 2026 07:52:54 +0100 Subject: [PATCH 3/3] Update docs --- docs/technical_settings.md | 25 +++++++++++++++++++++++-- 1 file changed, 23 insertions(+), 2 deletions(-) diff --git a/docs/technical_settings.md b/docs/technical_settings.md index 5e71402..461110e 100644 --- a/docs/technical_settings.md +++ b/docs/technical_settings.md @@ -9,6 +9,7 @@ lives and how to reach it. Everything about timing, retries, and conflict handli project makes rather than a dial to hand you (see [Sync](technical_sync.md)). - [What Is Stored](#what-is-stored) +- [Providers](#providers) - [Normalizing At The Point Of Use](#normalizing-at-the-point-of-use) - [Prefixes](#prefixes) - [The Secret](#the-secret) @@ -21,10 +22,10 @@ Settings persist to `data.json` in the plugin's own folder. | Field | Meaning | | ------------- | -------------------------------------------------------------- | -| `provider` | `r2` or `custom` | +| `provider` | `r2`, `s3`, or `custom` | | `accountId` | Cloudflare account, which R2 derives endpoint and region from | | `endpoint` | The S3 compatible endpoint, for a custom provider | -| `region` | The region, for a custom provider | +| `region` | The region, for Amazon S3 and a custom provider | | `bucket` | The bucket name | | `prefix` | The folder inside the bucket the vault lives under | | `accessKeyId` | The access key | @@ -35,6 +36,26 @@ to vault scoped localStorage instead, because settings travel to every device th `.obsidian/` folder and both of those are statements about one machine (see [Device](technical_device.md)). +### Providers + +A provider is only a way of arriving at an endpoint and a signing region. Everything past that point +is the same S3 API for all three. + +| Provider | Endpoint | Signing region | +| ------------------ | --------------------------------- | --------------------- | +| `r2` Cloudflare R2 | Derived from `accountId` | Always `auto` | +| `s3` Amazon S3 | Derived from `region` | The `region` as typed | +| `custom` | Typed in full | The `region` as typed | + +Custom only appears in development builds, where esbuild defines `NODE_ENV`. It exists for the local +MinIO setup contributors run, and a production user has no reason to reach for a raw endpoint field +when R2 and Amazon S3 both derive theirs. + +Amazon S3 puts the region straight into the endpoint host, so the region is the endpoint. A value +carrying URL authority delimiters, `x@attacker.example:443#`, would otherwise send signed requests +and vault data to a host nobody chose. So an unrecognised region yields no endpoint at all, and the +settings tab reports it the same way it reports a missing one. + ### Normalizing At The Point Of Use Endpoint, region, and prefix are stored exactly as typed and canonicalized where they are used, not