Skip to content

fix: anchor the https scheme downgrade in containerFetch - #250

Merged
mattjohnsonpint merged 1 commit into
mainfrom
mjp/fix-scheme-downgrade
Aug 24, 2026
Merged

fix: anchor the https scheme downgrade in containerFetch#250
mattjohnsonpint merged 1 commit into
mainfrom
mjp/fix-scheme-downgrade

Conversation

@mattjohnsonpint

@mattjohnsonpint mattjohnsonpint commented Aug 22, 2026

Copy link
Copy Markdown
Contributor

Summary

  • anchor the scheme rewrite so it only matches the URL's scheme
  • add regression coverage for https: in query strings and fragments
  • pin the intended scheme downgrade so it can't be dropped by a future fix
  • publish as a patch release

Why

containerFetch downgrades https to http before forwarding, because tcpPort.fetch opens a raw TCP connection to the container and nothing terminates TLS. That rewrite used an unanchored string replace:

const containerUrl = request.url.replace('https:', 'http:');

String.prototype.replace with a string pattern replaces only the first occurrence, and the match isn't anchored to the start. When the URL's scheme is already http:, the first https: in the string is somewhere in the path, query, or fragment — and that gets rewritten instead of the scheme.

So this:

await this.containerFetch('/callback?redirect=https://app.example.com');

arrives at the container as:

http://container/callback?redirect=http://app.example.com
                                        ^ silently downgraded

Any endpoint taking an unencoded absolute URL as a parameter is affected — OAuth redirect_uri, webhook callbacks, proxy targets, SSO RelayState. Percent-encoded values (https%3A%2F%2F…) are unaffected, since they don't contain the literal https:.

Absolute https:// URLs were never affected: the first match is the scheme, so the replacement did the right thing and the rest of the URL survived.

Relationship to #238

The bug is pre-existing, but #238 widened its reach considerably. Relative paths now resolve against http://container, which means every relative-path call has an http: scheme — precisely the condition that triggers this. Before #238, relative paths threw outright, so the corruption was largely unreachable.

Approach

Anchoring the regex is the whole fix. I also considered operating on the parsed URL:

const target = new URL(request.url);
if (target.protocol === 'https:') target.protocol = 'http:';

new URL(request.url).href round-trips request.url exactly (verified across default ports, empty paths, and percent-encoded characters), so the two are equivalent. The anchored regex wins on being a one-token change with no normalization risk and no extra allocation in the request-proxy hot path.

The comment above the line explains why the anchor matters, since the obvious "simplification" back to a string argument reintroduces the bug silently.

Tests

Two cases in src/tests/container.test.ts:

  • preserves https: in query strings and fragments — the regression. Fails on main, passes here.
  • downgrades only the scheme of an https URL — asserts the intended downgrade still happens and doesn't touch a https: query param on the same URL. This case was already correct; it's pinned so an over-eager future fix can't disable the downgrade entirely.

Only the first fails without the change, which is expected — the second documents behavior that already worked.

@mattjohnsonpint
mattjohnsonpint requested a review from a team as a code owner August 22, 2026 01:50
@pkg-pr-new

pkg-pr-new Bot commented Aug 22, 2026

Copy link
Copy Markdown

Open in StackBlitz

npm i https://pkg.pr.new/@cloudflare/containers@250

commit: 97781a2

@ask-bonk

ask-bonk Bot commented Aug 22, 2026

Copy link
Copy Markdown
Contributor

LGTM

github run

The scheme downgrade applied before forwarding to the container used an
unanchored string replace. String.prototype.replace rewrites only the
first match, so when the request URL was already http the first https:
in the path, query, or fragment was downgraded instead of the scheme.

This corrupts parameters carrying an absolute URL, for example
/callback?redirect=https://app.example.com, which arrives at the
container as redirect=http://app.example.com. Percent-encoded values
were unaffected.

Relative paths resolve against http://container, so every relative-path
call met the condition once #238 made them usable.

Anchoring the match leaves the intended https-to-http downgrade intact,
since tcpPort.fetch opens a raw TCP connection that does not terminate
TLS.
@mattjohnsonpint
mattjohnsonpint force-pushed the mjp/fix-scheme-downgrade branch from 617c08f to 97781a2 Compare August 24, 2026 04:31
@ask-bonk

ask-bonk Bot commented Aug 24, 2026

Copy link
Copy Markdown
Contributor

LGTM

github run

@mattjohnsonpint
mattjohnsonpint merged commit a17055c into main Aug 24, 2026
9 checks passed
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.

2 participants