Allow archive.org downloads in the browser app - #283
Merged
Conversation
Download & Run failed in the Avalonia Browser build for VisiCalc and Dangerous
Dave. Both are hosted on archive.org, which was not in the proxy's
ALLOWED_TARGET_HOSTS, so the worker answered 403 "Target host not allowed".
The other four Apple II entries use mirrors.apple2.org.za and were unaffected,
and the desktop app fetches directly so it never hit this.
Adding archive.org alone would not have fixed it. Every download there 302s to
a per-request node host - dn711007.ca.archive.org, ia801234.us.archive.org and
so on - and the worker re-validates each redirect hop against the same
allowlist, so the hop would have failed instead with "Redirect target host not
allowed". Those hostnames vary per request and cannot be enumerated.
isAllowedTargetHost now supports a "*." wildcard entry. Three choices worth
stating:
- The wildcard is opt-in per entry rather than implied by every entry, so
allowing subdomains of one host does not quietly widen the rest. csdb.dk
still means only csdb.dk.
- A wildcard matches subdomains only, so a host needing both is listed twice
(archive.org and *.archive.org). They are different trust decisions and the
allowlist should read literally.
- The compared suffix keeps its leading dot. Testing endsWith("archive.org")
would also admit evilarchive.org, which is an allowlist anyone can join by
registering the right name.
validateConfig now rejects wildcards too broad to be an allowlist - a bare "*",
or one with no dot after it such as "*.com". A bare "*" makes this an open
proxy, and that should fail at startup rather than be discovered from traffic.
Verified against the real service by running the worker locally: VisiCalc and
Dangerous Dave both return 143360 bytes - exactly a 140 KB DOS 3.3 image, and
VisiCalc starts 01 A5 27, the Apple II boot sector - while evilarchive.org and
archive.org.evil.com are still refused.
Also fixes a test that was already failing before this change: the health
document assertion still listed four allowed hosts after mirrors.apple2.org.za
was added as a fifth. It went unnoticed because no workflow ran these tests, so
this adds one, path-filtered to the worker. A suite nothing runs is not a gate.
20 tests pass.
SonarCloud flagged the new workflow: npm ci without --ignore-scripts lets a dependency's lifecycle scripts execute arbitrary code during install (githubactions:S6505, MAJOR). Worth taking seriously in this tree, which already carries seven high-severity advisories reached through wrangler. Nothing here needs those scripts. Verified by deleting node_modules, running npm ci --ignore-scripts, and getting all 20 tests passing from that clean install.
The quality gate failed on new_coverage 0% against a threshold of 80. The nine uncovered lines were the wildcard matcher and config guard in the CORS proxy Worker - which are covered, by six of that Worker's twenty tests. SonarCloud receives no coverage report for the TypeScript under tools/**, so it read "no data" as "untested". This gate measures the .NET product: sonar.coverage.inclusions already narrows it to a single library, and sonar.cpd.exclusions already carves out tools/** as "standalone TypeScript deployables (not the .NET product)". Coverage was simply never made to agree with that decision, because until now nothing under tools/** had changed in a PR. So this is the existing scope decision applied to a second metric, not a new exemption to dodge the gate. The Workers keep their own vitest suites and their own workflows; if their coverage is ever reported here, drop the exclusion and feed the lcov in instead. Noted in the comment.
CI / Documentation and the two CI / WASM AOT checks sat at "Expected - Waiting for status to be reported" on this PR, and would have sat there forever. Both workflows filter on `paths`. A workflow filtered out never runs, and one that never runs never reports a status - but branch protection still requires a status from it. So any PR touching none of the listed paths is unmergeable. It is a deadlock, not a slow check, and it stays invisible until a PR happens to miss every path in the list. This one is the first to touch only tools/** and .github/workflows/**; every previous PR touched src/** or docs/** and so satisfied both by accident. The fix is to let the jobs always run and always report, and move the path test inside the job: - The pull_request trigger loses its `paths` filter. The push trigger keeps its own, since nothing requires a status from a push. - A first step diffs against the PR base and sets an output. - Every subsequent step is gated on that output. A miss now costs a few seconds of runner time instead of a merge, and the expensive work - AOT publish, Playwright, mkdocs - is still skipped exactly as before. Job names are unchanged, which matters: the required checks are matched by name. The alternative, a second workflow duplicating the job names on inverse path filters, was rejected. It puts each required name in two files, so renaming one breaks every PR - the same failure this commit is removing. cors-proxy.yml gets the same treatment even though its check is not required. It was added in this PR with the same latent trap, and leaving it would mean marking it required later silently reintroduces the deadlock. Both trigger paths verified: on this PR the workflow files themselves changed, so all three checks report having done their real work.
|
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Download & Run failed in the Avalonia Browser build for VisiCalc and Dangerous Dave. Both are hosted on archive.org, which was not in the CORS proxy's
ALLOWED_TARGET_HOSTS, so the Worker answered403 Target host not allowed. The other four Apple II entries usemirrors.apple2.org.zaand were unaffected, and the desktop app fetches directly so it never hit this.Reproduced against the live Worker before changing anything, and the deployed vars matched
wrangler.jsonc, so there was no config/deployment drift.Why adding
archive.orgwas not enoughEvery archive.org download 302s to a per-request node host:
The Worker re-validates each redirect hop against the same allowlist, so allowing only
archive.orgwould have swapped one 403 forRedirect target host not allowed. Those node hostnames vary per request and region and cannot be enumerated.The change
isAllowedTargetHostgains a*.wildcard entry. Three choices worth stating:csdb.dkstill means onlycsdb.dk.archive.organd*.archive.org). They are different trust decisions and the allowlist should read literally.endsWith("archive.org")would also admitevilarchive.org— an allowlist anyone can join by registering the right name.validateConfignow rejects wildcards too broad to be an allowlist: a bare*, or one with no dot after it such as*.com. A bare*makes this an open proxy, and that should fail at startup rather than be discovered from traffic.Verification
Run against the real service with the Worker running locally, not just mocked:
01 A5 27evilarchive.orgarchive.org.evil.com143 360 is exactly a 140 KB DOS 3.3 image and
01 A5 27…is the Apple II boot sector, so that is the real file through the real redirect chain.Already deployed and verified manually in the browser app. The Worker deploy is independent of this merge — this PR is the source of record catching up with what is live.
20 tests pass, including 6 new ones: exact entries do not match subdomains, wildcards match node hosts, a wildcard alone does not cover the bare domain, the three lookalike cases, over-broad wildcards rejected, and a redirect-hop test mirroring archive.org's actual 302.
Two incidental fixes
mirrors.apple2.org.zawas added as a fifth. Confirmed by stashing and re-running..github/workflows/cors-proxy.yml, path-filtered totools/cloudflare/cors-download-proxy/**. A suite nothing runs is not a gate.Flagged, not fixed
npm auditreports 7 high-severity advisories, allundicireached viawrangler/@cloudflare/vitest-pool-workers. Dev/test dependencies, not shipped in the Worker; the fix needs--forceand a breaking bump, so it wants a decision rather than a silent change.