[#158] Settings → Restrict custom provider to local development

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

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",

src/settings/settings.test.ts

@@ -8,10 +8,12 @@ import {
   endpointFor,
   type GeodeSettings,
   hasConnectionConfig,
+  isAwsRegion,
   isCurrentConnectionResult,
   normalizePrefix,
   normalizeSettings,
   prefixError,
+  providerOptions,
   regionFor,
   saveDraft,
   settingsEqual,
@@ -198,9 +200,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",
@@ -266,6 +268,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" },
@@ -291,6 +298,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) {
@@ -299,6 +321,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",
@@ -392,6 +435,21 @@ for (const { name, input, want } of prefixErrorCases) {
   });
 }
 
+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",
@@ -453,6 +511,41 @@ 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: "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: {

src/settings/settings.ts

@@ -14,11 +14,14 @@ export const DEFAULT_SETTINGS: GeodeSettings = {
 // ConnectionStatus is the current in-memory state of a Test Connection check.
 export type ConnectionStatus = "unknown" | "checking" | "ok" | "error";
 
+// 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; see
 // docs/technical_settings.md for why each field is normalized where it is used rather than saved.
 export type GeodeSettings = {
   version: number;
-  provider: "r2" | "custom";
+  provider: Provider;
   accountId: string;
   endpoint: string;
   region: string;
@@ -86,11 +89,19 @@ export function normalizeEndpoint(endpoint: string): string {
   return normalized;
 }
 
-// endpointFor returns the storage endpoint URL to use for the given settings.
+// endpointFor returns the storage endpoint URL for settings, or "" when none can be derived; an
+// Amazon S3 region lands in the URL authority, so an unrecognised one yields no endpoint rather
+// than a host we never meant to sign a request against.
 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`;
+  }
 
   return normalizeEndpoint(settings.endpoint);
 }
@@ -103,9 +114,18 @@ export function hasConnectionConfig(settings: GeodeSettings): boolean {
   if (settings.provider === "r2") {
     return settings.accountId !== "";
   }
+  if (settings.provider === "s3") {
+    return isAwsRegion(settings.region);
+  }
   return settings.endpoint !== "" && settings.region !== "";
 }
 
+// isAwsRegion reports whether region looks like an AWS region identifier; only the restricted
+// alphabet matters for safety, since it admits no character that can redirect a URL authority.
+export function isAwsRegion(region: string): boolean {
+  return /^[a-z]{2}(-[a-z]+){1,2}-\d{1,2}$/.test(region);
+}
+
 // isCurrentConnectionResult reports whether a completed test still describes the current draft.
 export function isCurrentConnectionResult(
   checkId: number,
@@ -179,10 +199,19 @@ export function prefixError(raw: string): string {
   return "";
 }
 
-// providerOr returns "custom" if v is "custom", otherwise "r2".
-export function providerOr(v: unknown): "r2" | "custom" {
-  if (v === "custom") {
-    return "custom";
+// providerOptions returns user-facing providers, including Custom only for local development.
+export function providerOptions(localDev: boolean): Record<string, string> {
+  if (localDev) {
+    return { r2: "Cloudflare R2", s3: "Amazon S3", custom: "Custom" };
+  }
+
+  return { r2: "Cloudflare R2", s3: "Amazon S3" };
+}
+
+// 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";
 }

src/settings/tab.ts

@@ -18,6 +18,7 @@ import {
   hasConnectionConfig,
   isCurrentConnectionResult,
   prefixError,
+  providerOptions,
   providerOr,
   saveDraft,
   settingsEqual,
@@ -206,6 +207,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.")
@@ -266,7 +283,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);

src/storage/storage.test.ts

@@ -84,6 +84,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: {

src/storage/storage.ts

@@ -2,6 +2,7 @@ import { AwsClient } from "aws4fetch";
 import {
   endpointFor,
   type GeodeSettings,
+  isAwsRegion,
   normalizePrefix,
   prefixError,
   regionFor,
@@ -275,8 +276,8 @@ function conditionHeaders(condition: PutCondition | undefined): Record<string, s
 }
 
 // missingFieldFor returns the name of the first field testConnection needs but doesn't have, or
-// "" when everything required is present; R2 derives endpoint and region from the account ID, so
-// only custom needs them explicitly.
+// "" when everything required is present; R2 derives endpoint and region from the account ID and
+// Amazon S3 derives its endpoint from the region, so only custom needs both explicitly.
 function missingFieldFor(settings: GeodeSettings, secretAccessKey: string): string {
   if (settings.bucket === "") {
     return "bucket";
@@ -292,13 +293,21 @@ function missingFieldFor(settings: GeodeSettings, secretAccessKey: string): stri
     if (settings.accountId === "") {
       return "account ID";
     }
-  } else {
+    return "";
+  }
+
+  if (settings.provider === "custom") {
     if (settings.endpoint === "") {
       return "endpoint";
     }
-    if (settings.region === "") {
-      return "region";
-    }
+  }
+  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 "";