Repository navigation
fix(site): resolve asset URLs before trusting them - #9
Merged
Merged
Conversation
…ling a failed render I shipped this page claiming "every href is checked against this repo's own asset prefix". The check was a startsWith on the raw string, and that is not the same property. This URL starts with the prefix and still resolves outside the download path: https://github.com/DevCop95/pullgoscript/releases/download/v9/../../../evil -> https://github.com/DevCop95/evil Found by running the shipped file, not by reading it: site.js was exercised in node against a stub DOM with the real API payload, the real checksums.txt, and a hostile payload. Before this commit that hostile payload yielded two rows, one of them pointing at a path the comment claimed was impossible. Not exploitable as shipped, and worth saying plainly: browser_download_url comes from api.github.com over TLS, so forging it needs either the ability to write releases in this repo (in which case you would simply point the link wherever you wanted) or a broken TLS chain. Nothing a reader can be tricked into. It is a claim/code mismatch, which is the same defect class as the validHost comment fixed in #7, and the fix is two lines: resolve the URL and check the origin and the path it actually lands on. Anything that escapes after resolution is dropped, which fails closed — no row, and the caller falls back to the pinned list. Two user-visible strings were also wrong or imprecise: - "Could not read the release list automatically" was printed when the list had been read perfectly well and simply had no usable asset. It now says "Could not build the live table automatically", which is what happened. - The render guard said "the latest release has no assets" when the filter, not the release, was the reason. It now names the URL check. Re-verified after the change, same harness: the real payload still renders 7 rows with the amd64 SHA-256 matching the published checksums.txt, the hostile payload drops from 2 survivors to 1 (javascript: URL, foreign host and the traversal are all gone, and a raw HTML asset name is still written as text), and the 403 and empty-payload paths still show the pinned list. No href assigned in any scenario fails the prefix assertion.
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
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.
Found by running the shipped
site.jsin node against a stub DOM, with the real API payload, the realchecksums.txtand a hostile payload.The prefix check was a
startsWithon the raw string, so this survived it and resolved outside the download path:Not exploitable as shipped —
browser_download_urlcomes from api.github.com over TLS, so forging it needs repo write access or a broken TLS chain. It is a claim/code mismatch, same class as thevalidHostcomment in #7, and the fix resolves the URL and checks the origin and final path. Anything that escapes after resolution is dropped, which fails closed.Also fixes two strings that mislabelled what happened: "Could not read the release list" when the list had been read and simply had no usable asset, and "the latest release has no assets" when the URL filter was the reason.
Re-verified with the same harness: 7 rows and the correct amd64 SHA-256 on the real payload, hostile payload down from 2 survivors to 1, 403 and empty paths still fall back to the pinned list, no href fails the prefix assertion.