diff --git a/CHANGELOG.md b/CHANGELOG.md index d683bfb..c877cab 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -4,6 +4,19 @@ All notable changes to this project are documented here. The format is based on [Keep a Changelog](https://keepachangelog.com/en/1.1.0/), and this project adheres to [Semantic Versioning](https://semver.org/spec/v2.0.0.html). +## [Unreleased] + +### Fixed +- **A trailing root-label dot on an allow-list ENTRY no longer silently blocks everything.** + `isAllowedHost` normalized the incoming hostname (trim, case-fold, strip the trailing dot) but + only trimmed and case-folded the configured entries, so + `LEXWARE_UPLOAD_ALLOWED_HOSTS=sharepoint.com.` matched nothing and blocked the very host it was + meant to allow. Both sides now go through the same normalization, so they cannot drift again. This + is the same failure mode as the leading dot fixed in 0.1.12, arriving from the other end of the + name; raised by the review bot on [#39] and not carried over at the time. A double trailing dot is + deliberately NOT folded — that is an empty DNS label, i.e. a malformed name rather than another + spelling of the same one. + ## [0.1.13] Robustness fixes surfaced by a full security audit and code-review pass of the server. No diff --git a/src/uploads/fetch-url.ts b/src/uploads/fetch-url.ts index aec8e82..a1bcdb3 100644 --- a/src/uploads/fetch-url.ts +++ b/src/uploads/fetch-url.ts @@ -14,17 +14,31 @@ const DEFAULT_MAX_REDIRECTS = 3; */ export const DEFAULT_ALLOWED_HOSTS = ["sharepoint.com", "onedrive.live.com", "1drv.ms", "graph.microsoft.com"]; +/** + * A host name reduced to its comparable form: trimmed, case-folded, and with the + * trailing root-label dot removed (`example.com.` and `example.com` are the same name; + * `new URL()` keeps the dot, so it reaches here). + * + * Used on BOTH sides of the comparison in {@link isAllowedHost}. A normalization applied + * to only one side is one that fails whenever the *other* side carries the odd spelling. + * + * Exactly one trailing dot, not `/\.+$/`: a second dot makes an empty DNS label, so + * `example.com..` is not another spelling of the same name and must not be folded into + * one — this step exists to equate equivalent names, not to repair malformed ones. + */ +function normalizeHostForMatch(value: string): string { + return value.trim().toLowerCase().replace(/\.$/, ""); +} + /** * True when `hostname` is exactly one of `allowed`, or a subdomain of one of them * (matched on a dot boundary — "evilsharepoint.com" must NOT match "sharepoint.com"). - * Case-insensitive; a trailing dot on `hostname` (a valid DNS root-label terminator) is - * stripped before comparison. + * Case-insensitive, and a trailing root-label dot on either side is ignored. */ export function isAllowedHost(hostname: string, allowed: string[]): boolean { - let host = hostname.trim().toLowerCase(); - if (host.endsWith(".")) host = host.slice(0, -1); + const host = normalizeHostForMatch(hostname); for (const entry of allowed) { - const suffix = entry.trim().toLowerCase(); + const suffix = normalizeHostForMatch(entry); if (!suffix) continue; if (host === suffix || host.endsWith(`.${suffix}`)) return true; } diff --git a/tests/uploads-fetch-url.test.ts b/tests/uploads-fetch-url.test.ts index 7d0cb2d..aba1d0d 100644 --- a/tests/uploads-fetch-url.test.ts +++ b/tests/uploads-fetch-url.test.ts @@ -75,6 +75,31 @@ describe("isAllowedHost", () => { expect(isAllowedHost("FOO.SharePoint.COM", DEFAULT_ALLOWED_HOSTS)).toBe(true); expect(isAllowedHost("sharepoint.com.", DEFAULT_ALLOWED_HOSTS)).toBe(true); }); + + it("ignores a trailing root-label dot on a CONFIGURED ENTRY too, not only on the hostname", () => { + // The normalization used to run on the hostname alone, so an operator who wrote the + // fully-qualified form in LEXWARE_UPLOAD_ALLOWED_HOSTS silently blocked the very host + // they meant to allow — the same failure mode as the leading dot fixed in 0.1.12, + // arriving from the other end of the name. + expect(isAllowedHost("contoso.sharepoint.com", ["sharepoint.com."])).toBe(true); + expect(isAllowedHost("sharepoint.com", ["sharepoint.com."])).toBe(true); + // Odd spelling on both sides at once still resolves to the same name. + expect(isAllowedHost("contoso.sharepoint.com.", ["sharepoint.com."])).toBe(true); + expect(isAllowedHost(" SharePoint.COM. ", [" .. "])).toBe(false); + }); + + it("still refuses a lookalike when the entry carries a trailing dot — the dot is not a wildcard", () => { + expect(isAllowedHost("evilsharepoint.com", ["sharepoint.com."])).toBe(false); + expect(isAllowedHost("sharepoint.com.evil.com", ["sharepoint.com."])).toBe(false); + }); + + it("does not fold a DOUBLE trailing dot into the same name — that is an empty label, not a spelling", () => { + // `example.com..` is malformed rather than equivalent, and this step exists to equate + // equivalent names, not to repair broken ones. Widening it would quietly grow what the + // allow-list accepts. + expect(isAllowedHost("sharepoint.com..", DEFAULT_ALLOWED_HOSTS)).toBe(false); + expect(isAllowedHost("sharepoint.com", ["sharepoint.com.."])).toBe(false); + }); }); describe("fetchRemoteFile", () => {