Repository navigation
Commit 5c9bce0
committed
fix(registry): #103 review — a redirect is followed by hand, and only to where the registry keeps its files
Two findings from the review. One was right, and wider than it said. The other
was a true sentence in the doc that nobody could check from this repository.
1. The registry check went wherever it was redirected. It refused to START a
request anywhere but the registry, and then handed the request to fetch(),
which follows a redirect anywhere. The comment beside it — "only addresses
on the registry itself are followed" — was not true, and not only under
attack: Open VSX answers every download with a 302 to its content host, so
on every ordinary run each package and each signature archive came from
openvsx.eclipsecontent.org, an origin the check had never been told about.
Watching the reviewed code against the real registry shows it: of five
requests sent to open-vsx.org, four were answered by the other host.
"Reject redirects", the first remedy offered, would have left the check
unable to download anything. Redirects are followed by hand instead
(redirect: 'manual'): each destination is checked BEFORE anything is sent to
it, and passes only if it is the registry itself or a host named in
CONTENT_ORIGINS — one entry, the content host — at most three redirects
deep. The list is written down rather than learned from the answer, for the
reason the key is: a host the check is merely told about is a host anyone
who can answer for the registry could choose.
The price is pinning's price, paid the same way. The day Open VSX moves its
files, nothing changes for users and this check can no longer download
anything. It then says where the registry redirects and what to change:
"could not check", which is a warning in the release gate and a failed run
in the daily watch after three tries. docs/EXTENSION-SIGNATURES.md has a
section for that day.
Nothing caught this because the stand-in registry in the suite served files
directly: it did not behave like the registry it stood in for. It now
answers a download with a redirect to a content host, as Open VSX does, and
like fetch() it follows a redirect itself unless told not to — so a caller
that forgets to say so is seen going where it is sent.
2. "The editor checks [.signature.p7s] exists." It does, but not in any code
in this repository, and the verifier here does not; the reviewer could see
only the half that says no such thing. The editor's downloader looks for
the entry before it hands an archive to any verifier
(downloadSignatureArchive in extensionDownloader.ts, Code-OSS 1.126) and
discards an archive without it as a failed download. The doc now says who
checks, where, and that the module does not read the entry; a case in the
suite pins that an archive without it is not the module's to refuse.
Nothing changed in behaviour for this one. A limit it brought out is
written down too: the daily watch verifies with the module, so what only
the editor asks of an archive is covered by `smoke`, at release time.
Run against open-vsx.org after the change: every request leaves in manual mode
and is answered by the host it was sent to; status ok, two packages verified.
With the content host taken off the list: "could not check", the message names
openvsx.eclipsecontent.org, and only open-vsx.org was contacted.
Tests: 56 to 59 cases. The three new ones fail on the reviewed script; the
other 56 pass on it. 19 mutations of the new code each fail a case. 50 suites
pass on macOS (Node 24) and in a Linux container (Node 18).1 parent d4d58f2 commit 5c9bce0
5 files changed
Lines changed: 241 additions & 32 deletions
File tree
- docs
- modules/extension-signature
- test
- scripts
| Original file line number | Diff line number | Diff line change | |
|---|---|---|---|
| |||
104 | 104 | | |
105 | 105 | | |
106 | 106 | | |
| 107 | + | |
| 108 | + | |
107 | 109 | | |
108 | 110 | | |
109 | 111 | | |
| |||
| Original file line number | Diff line number | Diff line change | |
|---|---|---|---|
| |||
1 | 1 | | |
2 | 2 | | |
3 | 3 | | |
4 | | - | |
5 | | - | |
| 4 | + | |
| 5 | + | |
6 | 6 | | |
7 | 7 | | |
8 | 8 | | |
| |||
30 | 30 | | |
31 | 31 | | |
32 | 32 | | |
33 | | - | |
| 33 | + | |
34 | 34 | | |
35 | 35 | | |
36 | 36 | | |
| |||
94 | 94 | | |
95 | 95 | | |
96 | 96 | | |
97 | | - | |
| 97 | + | |
| 98 | + | |
| 99 | + | |
| 100 | + | |
| 101 | + | |
| 102 | + | |
| 103 | + | |
98 | 104 | | |
99 | 105 | | |
100 | 106 | | |
| |||
218 | 224 | | |
219 | 225 | | |
220 | 226 | | |
| 227 | + | |
| 228 | + | |
| 229 | + | |
| 230 | + | |
| 231 | + | |
| 232 | + | |
| 233 | + | |
| 234 | + | |
| 235 | + | |
| 236 | + | |
| 237 | + | |
| 238 | + | |
| 239 | + | |
| 240 | + | |
| 241 | + | |
| 242 | + | |
| 243 | + | |
| 244 | + | |
| 245 | + | |
| 246 | + | |
| 247 | + | |
| 248 | + | |
| 249 | + | |
| 250 | + | |
| 251 | + | |
| 252 | + | |
| 253 | + | |
| 254 | + | |
| 255 | + | |
| 256 | + | |
| 257 | + | |
| 258 | + | |
221 | 259 | | |
222 | 260 | | |
223 | 261 | | |
| |||
261 | 299 | | |
262 | 300 | | |
263 | 301 | | |
| 302 | + | |
| 303 | + | |
| 304 | + | |
264 | 305 | | |
265 | 306 | | |
| Original file line number | Diff line number | Diff line change | |
|---|---|---|---|
| |||
15 | 15 | | |
16 | 16 | | |
17 | 17 | | |
18 | | - | |
19 | | - | |
| 18 | + | |
| 19 | + | |
| 20 | + | |
20 | 21 | | |
21 | 22 | | |
22 | 23 | | |
| |||
0 commit comments