mirror of
https://github.com/backnotprop/plannotator.git
synced 2026-09-14 14:17:26 +08:00
Sun
f7068ce31e
* fix(pr-artifacts): read GitLab uploads through the token-readable API GitLab serves `/uploads/<secret>/<file>` from a Rails web route that only honors session cookies. A `PRIVATE-TOKEN` request is redirected to the sign-in page, so every GitLab upload attachment referenced by an MR was unreadable — and because the sign-in page is HTML served with HTTP 200, it was rendered as the artifact instead of the file. Route upload links through `GET /projects/:id/uploads/:secret/:filename`, which serves the same bytes for a personal access token. Both the bare `/uploads/...` and project-scoped `/<path>/uploads/...` forms are rewritten. Also stop following redirects into a provider sign-in path: fail with 401 and the matching `gh`/`glab auth login` hint so an auth gap is legible instead of surfacing as a corrupt artifact. The uploads API answers every file with `application/octet-stream`, and the content route serves provider types with `nosniff`, so refine an opaque type from the file extension to keep images and video rendering. Verified end-to-end against a self-hosted GitLab 18.8.0-ee instance. * fix(pr-artifacts): rewrite only real GitLab upload paths The upload rewrite matched `/uploads/<anything>/<anything>`, so a link in an MR body could name a path GitLab never minted and still be rewritten into `/api/v4/projects/:id/uploads/...` with the PRIVATE-TOKEN attached. Because the remainder allowed slashes, a crafted link could append an attacker-chosen path to that credentialed GET, leaving the safety of the request to GitLab's router rather than to this allowlist. Pin the shape GitLab actually mints: a 32 lowercase hex secret and a single filename segment. A non-conforming path is no longer rewritten, so it is fetched verbatim as the ordinary web route exactly as before this feature. Test fixtures move to a real 32-hex secret, and a new case pins that a short secret, an uppercase secret, and a multi-segment remainder are all left alone. * fix(pr-artifacts): diagnose a direct provider refusal as a missing login The sign-in guard only fired on a redirect, but GitLab's `/api/v4` routes refuse a bad or absent token directly with a 401/403 JSON body. Those fell through to `Artifact host returned HTTP 401` at status 502, which reads as a broken artifact rather than the thing the reader can fix. Hoist the sign-in messaging into `providerAuthRequiredError` and reuse it for a direct refusal, so both paths produce the same actionable error at status 401. GitHub's 403 stays a transport status on purpose: it also covers rate limiting and SSO enforcement, where a `gh auth login` hint points at the wrong problem. A GitHub 401 is unambiguous and is diagnosed. The message text moves to a colon instead of a dash to match house style. * fix(pr-artifacts): fall back to the upload web route when the API route is absent `GET /projects/:id/uploads/:secret/:filename` landed in GitLab 17.4. On an older self-hosted instance the rewrite turns an upload that used to load, on a public project where the web route reads anonymously, into a 404 surfaced as a 502. That is a regression the rewrite introduced. When the rewritten uploads API URL answers 404, retry the original web route once before failing. The retry is deliberately narrow: only for the upload rewrite, only on 404, and only when the 404 came from the rewritten URL itself, with no version probing anywhere. It reuses the loop's redirect budget, so the total number of requests stays bounded, and it is same-origin, so credentials are attached on exactly the existing `shouldSendProviderAuth` terms. A sign-in redirect from that retry still produces the actionable 401, so a private project on an old instance reports a missing login rather than a 404. * fix(pr-artifacts): drop the active content types from the extension map `html` and `text/javascript` bought nothing. An HTML artifact is read through `/api/pr-artifact-document`, which always answers `text/plain; charset=utf-8`, so nothing on that path ever consulted this map. Keeping them meant the media route could label an upload as active content, leaving safety resting on the `Content-Security-Policy: sandbox` header staying in place forever. `css` stays because `shouldRewriteCss` keys on `text/css` to rewrite provider references, and the image and video types stay because `nosniff` means an unrefined octet-stream simply does not render. `svg` stays too, on the evidence of how svg uploads actually reach the screen. `.svg` is not in the review editor's IMAGE_EXTENSIONS, so an svg upload arrives either as an authored markdown image, rendered through `<img src>`, which is a non-scripting context by spec, or as a resource referenced from an HTML artifact, which renders inside an `<iframe sandbox="">`. Both are served by the media route with `Content-Security-Policy: sandbox` and `X-Content-Type-Options: nosniff`, so even a direct navigation to the proxy URL lands in a sandboxed, opaque-origin document that cannot run script. Dropping it would give up real rendering for no reduction in reachable capability. Adds a regression test pinning that an .html or .js upload stays opaque. * test(pr-artifacts): pin that the upload rewrite does not widen token reach The rewrite sends PRIVATE-TOKEN to a different path on the provider origin, so the invariant worth guarding is that it did not also change where that token can travel. GitLab object storage answers an upload with a 302 to a signed URL on an unrelated host, which is exactly the hop a credential must not follow. Verified the test bites: forcing shouldSendProviderAuth to return true for gitlab fails it on the second request's header. The module-global auth cache has a 5 minute TTL and no reset seam, so the test resolves the same `test-token` every other gitlab case in this file resolves, which makes it correct whether the cache is cold or warm rather than dependent on test order. --------- Co-authored-by: Sun Neoh <yuensun.neoh@stashaway.com> Co-authored-by: Michael Ramos <mdramos8@gmail.com>