Skip to content

fix(uploads): normalize allow-list entries the same way as the hostname - #42

Merged
marselsel merged 1 commit into
marselsel:mainfrom
gutencoder:fix/allowlist-trailing-dot
Aug 20, 2026
Merged

fix(uploads): normalize allow-list entries the same way as the hostname#42
marselsel merged 1 commit into
marselsel:mainfrom
gutencoder:fix/allowlist-trailing-dot

Conversation

@gutencoder

Copy link
Copy Markdown
Contributor

A one-line follow-up to something your review bot spotted on #39 that did not get carried
across when the fixes were applied.

The bug

isAllowedHost normalizes the incoming hostname three ways — trim, case-fold, strip the
trailing root-label dot — but the configured entries only two. The missing step is the dot:

LEXWARE_UPLOAD_ALLOWED_HOSTS=sharepoint.com.
  → entry stays "sharepoint.com."
  → never equals "acme.sharepoint.com", never a dot-boundary suffix of it
  → the host the operator wrote down is the one host that is blocked

new URL("https://acme.sharepoint.com./f.pdf").hostname is "acme.sharepoint.com." — the
dot survives parsing, which is exactly why the hostname side needed the step. The entry
side needs it for the same reason.

This is the same failure mode as the leading dot you fixed in 0.1.12, arriving from the
other end of the name: a fully-qualified spelling that is silently inert instead of
either working or complaining.

The fix

One normalizeHostForMatch applied to both sides, so they cannot drift apart again.
A normalization applied to one side only is one that fails whenever the other side
carries the odd spelling.

Two judgement calls worth stating

The leading-dot strip stays in config.ts. I did not move or duplicate it. A leading
dot is an input convention — the cookie/Java .sharepoint.com form — and belongs to the
layer that accepts operator input. A trailing dot is DNS root-label equivalence and
belongs to the matcher, which already applied it to the hostname. Different things, so
different homes; the alternative would leave two places both half-responsible for the same
question.

Exactly one trailing dot, not /\.+$/. A second dot makes an empty DNS label, so
example.com.. is a malformed name rather than another spelling of the same one. This
step exists to equate equivalent names, not to repair broken ones — folding doubles would
quietly widen what the allow-list accepts, which is the wrong direction for this
particular function. There is a test pinning that sharepoint.com.. stays refused, from
both sides.

Verification

  • npm test328 tests / 19 files, up from 325.
  • npm run build and docker build pass. No dependency change.
  • I checked the new test fails on the unpatched matcher, so it measures the change
    rather than agreeing with it.

No behaviour change for any allow-list that does not carry a trailing dot — including the
built-in default, which is why nothing in the existing suite moved.

`isAllowedHost` normalized the incoming hostname three ways — trim, case-fold,
strip the trailing root-label dot — but the configured entries only two. The
missing step was the dot, so `LEXWARE_UPLOAD_ALLOWED_HOSTS=sharepoint.com.`
matched nothing and silently blocked the very host the operator wrote down.
`new URL()` keeps a trailing dot in `hostname`, which is why the hostname side
needed the step in the first place; the entry side needs it for the same reason.

Both sides now go through one `normalizeHostForMatch`, so they cannot drift
apart again — a normalization applied to one side only is one that fails
whenever the OTHER side carries the odd spelling.

This is the same failure mode as the leading dot fixed in 0.1.12, arriving from
the other end of the name. The review bot raised it on marselsel#39 and it was not
carried over at the time.

Left in config, deliberately: the leading-dot strip stays where it is. A leading
dot is an input CONVENTION (the cookie/Java `.sharepoint.com` form) and belongs
to the layer that accepts operator input; a trailing dot is DNS root-label
EQUIVALENCE and belongs to the matcher, which already applied it to the hostname.

Exactly one trailing dot, not /\.+$/: a second dot makes an empty DNS label, so
`example.com..` is a malformed name rather than another spelling of the same
one. Folding it would quietly widen what the allow-list accepts. A test pins
that it stays refused, on both sides.

npm run build, npm test (328 tests / 19 files, up from 325) and docker build
pass. Verified the new test fails on the unpatched matcher — it measures the
change rather than agreeing with it.
Copilot AI lite review requested due to automatic review settings August 19, 2026 12:47
@gutencoder
gutencoder requested a review from marselsel as a code owner August 19, 2026 12:47

Copilot AI left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

Pull request overview

Aligns upload URL allow-list matching so configured host entries are normalized exactly the same way as the incoming hostname, preventing fully-qualified (trailing-dot) entries from becoming inert and unexpectedly blocking intended hosts.

Changes:

  • Introduces a single normalizeHostForMatch() helper and applies it to both hostname and allowed entries in isAllowedHost.
  • Adds regression tests covering trailing-dot behavior on configured entries, plus negative cases (lookalikes and double trailing dots).
  • Documents the fix in CHANGELOG.md under Unreleased.

Reviewed changes

Copilot reviewed 3 out of 3 changed files in this pull request and generated no comments.

File Description
src/uploads/fetch-url.ts Centralizes hostname normalization and uses it on both sides of allow-list comparisons.
tests/uploads-fetch-url.test.ts Adds focused tests to pin configured-entry trailing-dot behavior and related edge cases.
CHANGELOG.md Notes the allow-list trailing-dot fix and its intended non-behavior for malformed double-dot cases.

💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.

@marselsel
marselsel merged commit 5c02470 into marselsel:main Aug 20, 2026
2 checks passed
@marselsel

Copy link
Copy Markdown
Owner

Merged, thank you! First contribution here that went through every review layer without a single finding: I verified the bug on the unpatched matcher, checked the fail-closed edges (double dot refused on both sides, no wildcard behaviour, lookalikes still rejected), and confirmed your mutation claim locally (reverting the entry-side normalization fails exactly your new test).

Both judgement calls read right to me, and the reasoning for them is the part I appreciate most: leading dot as input convention in config, trailing dot as DNS equivalence in the matcher, and refusing to fold a malformed double dot rather than quietly widening the list. Rides on main until the next release. That is four for four now.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants