Repository navigation
Gate the raw HTML that reaches the page - #194
Merged
Merged
Conversation
The agenda read three front matter fields through safeHTML: python_level and topic_level in agenda.html and event_detail.html, and legend in agenda.html. safeHTML bypasses escaping even when goldmark unsafe is off, so any merged pull request could put script into every agenda page. The site takes talk data from outside contributors, so this was a persistent cross-site scripting path. Front matter now carries tokens and the templates own the markup: - python_level and topic_level take beginner, intermediate or advanced. A new level_badge.html partial maps the token to a label and a class. An unknown token prints as escaped text and logs a build warning. - legend is a list of items: "type: <token>", "level: <token>" or "separator: true". A new agenda_legend.html partial renders it. - event_types.html holds the one table of type tokens, icons and labels. event_type_icon.html renders an icon from it. This replaces the two copies of the if-else icon chain in agenda.html and event_detail.html, which had drifted: the modal was missing the "photo" type. The change also fixes two accessibility defects: - The level was given by the colour of a dot alone, which fails WCAG 1.4.1 Use of Color. The badge now shows the text label next to the dot. - The dots did not meet the 3:1 floor of WCAG 1.4.11 Non-text Contrast. A red dot on the red cell of the agenda gave 1.38:1. The badge now draws its own white background, so contrast no longer depends on the cell colour: beginner 5.1:1, intermediate 5.2:1, advanced 5.5:1. The badge border holds 3:1 or better against every cell colour. - The legend was set to "opacity: 0.7" with full opacity on hover, which cut the contrast for every reader who does not hover. Removed. - The gold star of a sponsored event gave 1.44:1 on the white page background. Darkened to #8C6900 inside the legend. Migrated every affected file in content/events: 156 HTML level values in 6 files, 19 plain text level values in pyday_bcn_2020.md, and 6 legend strings. The legend of pyday_bcn_2024.md had a copy and paste fault, a second escaped icon where the word "Beginner" belonged. The structured legend removes it. Build verified: 310 HTML pages, no warnings from the new partials.
The site merges pull requests from outside contributors. A speaker edits their own page under content/people/, which stays unowned so that it needs no organizer. The paths in this file are different: a change there reaches every page, or reaches money, or reaches the deploy. Owned paths: .github/, bin/, config.toml, layouts/, static/, themes/, content/pybcn_association/ and CNAME. layouts/ is covered because layouts/shortcodes/membership/membership-process.html holds the PayPal hosted_button_id of the association, 6XH6B9ZGJ4PQA. One changed character in that value sends the membership fees to another PayPal account, and the rendered page still looks correct. CNAME is not in the original list. It is added because it sets the custom domain of the GitHub Pages site, so a change to it points pybcn.org somewhere else. Owner is @pybcn/core. Verified read only through the GitHub API: the team exists, has 17 members, and has push and admin permission on pybcn/pybcn.github.io, so it is a valid code owner. The team privacy is "secret"; if review requests do not reach it, make the team visible. CODEOWNERS only requests review. It has no effect until branch protection on the default branch requires review from code owners. Build verified: 310 HTML pages.
The site merges markdown from outside contributors and goldmark renders raw HTML, so a content change can carry script, a redirect or a fake form. Nothing checked for that. bin/check-html-safety scans content/ and exits 1 when it finds <script, <iframe, an on...= event handler, a javascript: URL, <object, <embed, <form, <meta, <link or <base. It reads each file twice over: - The YAML front matter as raw text, which gives true line numbers, and again as a parsed value tree, which covers nested lists and maps and catches a string that only becomes dangerous after YAML decodes it, such as "<scr\x69pt>". Neither pass alone is enough. The two are paired per pattern so that one problem is reported once. - The markdown body. The on...= pattern carries a word boundary. Without it the pattern also matches the middle of ordinary words in prose. Three iframes in the tree are legitimate, so ALLOWLIST holds one entry each with the reason: - content/events/pyday_bcn/pyday_bcn_2019.md, the PyBCN public Google Calendar that shows the agenda of that past edition - content/pybcn_association/propose-a-talk.md, the talk proposal Google Form - content/pyladies_bcn/call-for-proposals.md, the PyLadies BCN call for proposals Google Form There is no <embed in content/. The only match for the word "embed" is the path of the Google Calendar URL in the first file above. An allowlist entry names the file, the pattern and a substring, and it is matched against the single HTML tag that the pattern hit, not against the whole file. A first draft matched the whole file, which let a second, hostile iframe ride along inside a file that already had an allowed one. Tested: an injected iframe next to the allowed calendar iframe fails the check. The script also reports an allowlist entry that matches nothing, so a stale exception gets removed. The existing workflow only runs after a merge to edition. The new content-check workflow runs the script on every pull request, which is where the problem has to be caught. This workflow was not asked for. Drop the file if a separate check job is not wanted. Verified: passes on the current tree, and fails with exit 1 on a fixture that carries all ten patterns in front matter, in nested front matter and in the body. Build unchanged at 310 HTML pages.
Work in progress, committed so it is not lost. The site builds and the safety check passes, but the goal of this change is not reached yet. What is done: - 13 shortcodes under layouts/shortcodes/ plus a render-link hook, covering the patterns the content actually used: grids, columns, alerts, icons, logo rows, prose blocks, schedules, the membership links, and Google embeds. - 24 content files converted from raw HTML to those shortcodes, including both Google Form embeds, which are now a google-embed shortcode. - shortcodes.scss added and imported, so the converted markup keeps its styling. What is NOT done: - config.toml still sets unsafe=true. Two files still carry raw HTML that needs a page redesign rather than a shortcode: content/events/pyday_bcn/pyday_bcn_2019.md and pyday_bcn_2018.md, which are pre-Hugo programmes pasted in as tables and sponsor logo blocks. Converting them is a content decision, not a mechanical one, so unsafe stays true until somebody makes it. - bin/check-html-safety reports two stale allowlist entries, for the two Google Forms that are now shortcodes. Harmless, and a one-line cleanup each. So the XSS surface is narrower but not closed. The three safeHTML sinks are gone (previous commit) and the dangerous-HTML check now runs on pull requests, which together stop a malicious person page from executing. Setting unsafe=false is the remaining step. Verified: build exits 0 with 310 HTML pages, and bin/check-html-safety passes.
The check arrived with its own workflow, content-check.yml, which duplicated the checkout and the Python setup of the pr-checks content job and ran on both pull_request and push. It is now a step of that job instead. One workflow, one checkout, and the two content checks next to each other. content-check.yml also used unpinned actions on an older runner image, so retiring it removes the last floating action references in the repository.
The check read as a blocklist of exact strings, so several shapes of the same markup passed: - <script> matched only the lowercase literal with a following space. <SCRIPT/src=...> and a tag broken across a newline did not match. - <style> was not checked at all. An inline stylesheet can cover the page. - The event handler pattern missed handlers written with no space before the equals sign, and matched inside ordinary words in prose. - javascript: was matched before entity decoding, so java	script: passed. A hand-written entity table is the wrong tool here: the entity is 	 with a capital T, and the first attempt at this used a lowercase key and let the payload through. It now decodes with html.unescape. - The allowlist was matched against the whole tag, so an allowed substring anywhere in it cleared the tag. It now matches only the value of src, href, data, or action. The check-content half of the original change waits for the image move, so it is in the accessibility and images pull request.
Two templates built markup with printf and then marked it safe, which switches off the contextual escaping of Go templates: - render-link.html quoted a link title with %q. That is shell-style quoting, not HTML escaping. A quote inside the title came back as a backslash plus quote, which closes the attribute early and leaves the rest of the title as stray attribute names. - head.html built the feed <link> element with printf and safeHTML, interpolating the site title. Both now write the markup in the template, so Go escapes each value for its context. The rendered output is unchanged except that values are now escaped: a link title with an apostrophe emits '. This removes the last uses of the printf plus safe-output pattern. The only safeHTML left is a literal HTML comment.
The file collector only read .md, .markdown, .html and .htm. Hugo also renders .org, .adoc, .rst and .pandoc content, so a contributor could add content/x.org with an #+BEGIN_EXPORT html block that holds <script>, and the check reported PASS without opening the file. The payload reached the built page. The collector now classifies every file under content/ by extension: - a known text format (markdown, html, org, asciidoc, rst, pandoc, txt and svg) is read and scanned in full - a known binary asset (png, jpg, gif, webp, ico, pdf) is skipped and counted - any other extension fails the check with a message that names the file and the two lists to update The exit path is a single failed flag, so unknown files, findings and read errors are all reported in one run.
…oads allowed() collected URLs from src, href, data and action and cleared the tag when any of them held the permitted substring. A browser loads an iframe from src only, so <iframe src="https://evil.example/phish" data="https://calendar.google.com/calendar/embed?"> passed the check and reached the page. This is the same class that commit 8c7aaa9 closed for title=, reopened through another attribute. LOAD_ATTRIBUTES now maps each element to the attributes the browser reads (iframe: src and srcdoc, img: src and srcset, form: action, object: data, a and link: href, embed and script: src) and the entry is matched there and nowhere else. Two related gaps in the same function are closed with it: - the match is on the prefix of the URL and not on a substring, so a permitted URL appended to a hostile one does not count - only the first occurrence of an attribute counts, as in the HTML parser, so <iframe src=evil src=permitted> is not cleared - every loaded URL must match, so a permitted src next to a srcdoc is not cleared either The allowlist key is renamed from contains to prefix to say what it does.
The check never looked at srcdoc. An iframe such as
<iframe srcdoc="<script>alert(document.domain)</script>"
src="https://calendar.google.com/calendar/embed?">
passed, because the allowlist cleared the frame on its src, and reached the
page. The browser prefers srcdoc over src, and a srcdoc document runs in the
origin of the embedding page, so this is script execution on the site.
A srcdoc pattern now reports the attribute wherever it appears. Nothing in
content/ uses it. It has no LOAD_ATTRIBUTES entry, so no allowlist entry can
clear it.
A style attribute was not checked at all, so
<div style="position:fixed;top:0;left:0;width:100vw;height:100vh;
background:#fff;z-index:9999">
with a link inside passed and rendered: a full page overlay over a site
that has a real membership payment flow, with no script involved. The
scheme check also matched javascript: only.
The style-attr pattern reports every style attribute whose value is not
only sizes and margins. SAFE_STYLE accepts width, height, margin and its
four sides, and border-width, each with a non-negative length, a percentage
or auto, and nothing else: no position, no z-index, no background, no
url(), no comment, no entity, no negative number, no viewport unit. The
archived PyDay 2018 and 2019 pages size their sponsor logos with such
values, and they stay as they are. They cannot be converted to width= and
height= attributes without a visual change, because the theme rule for
images in a column sets height to auto and a stylesheet rule beats a
presentational attribute.
vbscript: is matched like javascript:, on the normalised text. data: is
matched when a media type with a slash and a comma follow, which is the
shape of a data: URL that carries a document. The word data: in prose or a
YAML key is not a finding.
…shortcode The PyDay 2019 page carried the one live entry of the ALLOWLIST: a raw Google Calendar iframe. The page is not in .github/CODEOWNERS, so any contributor could edit it, and the allowlist entry is what made the data= and srcdoc bypasses reachable from a content pull request. The iframe is now a google-embed shortcode call. The shortcode lives in layouts/, which @pybcn/core owns, and it rejects any src that does not start with https://calendar.google.com/calendar/embed. The rendered src is the same URL. The frame gains a title, lazy loading and a referrer policy, and takes the column width instead of a fixed 800px. The ALLOWLIST is empty. The comment above it says to prefer the shortcode route, and that a future entry needs a CODEOWNERS rule for its file. border-width leaves SAFE_STYLE: the iframe was its only user.
bin/check-html-safety had a truncated comment inside the ALLOWLIST literal, left by an earlier edit. It is gone. README.md said the workflow was content-check and that three Google iframes sat in the allowlist. The step runs in the pr-checks workflow, in .github/workflows/pr.yml, and the allowlist is empty since the last embed moved to the google-embed shortcode. The section now lists every pattern the check has, says that it reads every content format Hugo renders and fails on an unknown extension, and says that an allowlist entry needs a CODEOWNERS rule for its file.
The docstring said the check reads the markdown body and listed two exit conditions. It now says that every content format Hugo renders is read, that an overlay counts as harm, and that an unknown extension is a failure.
A javascript:, vbscript: or data: match is found in the normalised text, which has every newline stripped so that a scheme split across a line still matches. The line number was then counted in that text, so every scheme finding reported the line the body starts on. Four findings on four different lines all said line 4. normalise_scheme now also returns the offset in the original text that produced each normalised character, and the finding is reported at that offset. The excerpt comes from the original text too, which is what the docstring already claimed. No change to what is detected: the same payloads fail, and the clean tree still passes.
The style grammar accepts width, height and margin so that the logo rows on the PyDay pages keep working. The length was unbounded, so an accepted property could still deface a page: style="width:99999px" passes the grammar and pushes everything else off the viewport. Four digits is the cap the logo shortcode already puts on its own width and height. The largest value any content file uses is 150px, so there is room to spare: all 18 distinct style values in content still pass.
Two changes to CODEOWNERS. The owner moves from @pybcn/core to @pybcn/web. @pybcn/core is a secret team, and GitHub does not send a review request to a secret team, so every rule in this file was silently reaching nobody. @pybcn/web is visible ("closed"), has push access, and holds the five people who maintain the site. The file already warned about this; it is now not needed. content/sponsors/ and content/events/ become owned. A sponsor file carries the name and the logo that render in the sponsor grid, and an event file decides which sponsors and which people a page lists. A review pass showed these are the paths a hostile sponsor entry comes through, and they were the only rendered paths outside both this file and the content check. This file still has no effect until "Require review from Code Owners" is enabled on the edition branch. That is a repository setting, not a change here.
This was referenced Oct 4, 2026
ifosch
reviewed
Oct 5, 2026
| if entry["pattern"] != pattern_name: | ||
| continue | ||
| if any(entry["contains"] in u for u in urls): | ||
| if all(url.startswith(entry["prefix"]) for url in urls): |
ifosch
approved these changes
Oct 5, 2026
ifosch
left a comment
ifosch
left a comment
Member
There was a problem hiding this comment.
What a huge piece of work! Thanks!
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
2 participants
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.
Stacked on #193. Its base is
pr/2-checks, so the diff shows only this pull request's own changes. GitHub retargets it as the ones below merge.The problem
content/accepts raw HTML, anyone can open a pull request against it, and[markup.goldmark.renderer] unsafe=trueis set, so that HTML renders verbatim. There is nothing between a contributed<script>and the deployed page.unsafe=truecannot simply be turned off: the PyDay 2018 and 2019 pages are built from pasted HTML tables, and that is a content decision for another day.Three layers
bin/check-html-safetyruns on every pull request and refuses 15 shapes of dangerous markup incontent/. Anything legitimate goes through a shortcode inlayouts/, which an organizer reviews, or through an allowlist entry with a written reason. A stale allowlist entry is reported so that it gets deleted.CODEOWNERSon the paths where a change does the most damage.The raw HTML already in content converted to shortcodes, with the agenda levels and the legend rendered from data instead of assembled as a string and marked safe.
Two template fixes ride along, because both are the same mistake: building markup with
printfand then marking it safe, which switches off the contextual escaping of Go templates.render-link.htmlquoted a link title with%q, which is shell quoting and not HTML escaping.head.htmlbuilt the feed<link>element the same way. Both now write the markup in the template.What review changed, and this is most of the pull request
A security review found four ways to pass the check and still reach the live page. Each was reproduced first: the checker printed PASS and the payload appeared in the built HTML. All four are closed, and each fix is verified with the payload that defeated the previous version.
The extension list was fail open. The checker read
.md,.markdown,.htmland.htm. Hugo renders more content formats than that, socontent/x.orgwith an#+BEGIN_EXPORT htmlblock carrying a<script>executed on the page and the checker never opened the file. It now scans every content format Hugo has registered, skips a known list of binary assets, and fails on any extension it does not recognise rather than ignoring it..svgis scanned as text, because it is XML that can carry a script and it is served verbatim.The allowlist cleared a tag by OR-ing across attributes. It collected URLs from
src,href,dataandactionand cleared the tag if any one of them matched. A browser loading an<iframe>uses onlysrcandsrcdoc, so<iframe src="https://evil.example/" data="https://calendar.google.com/calendar/embed?">was accepted. The match is now against the attribute the element actually loads from, per element, as a URL prefix rather than a substring, counting only the first occurrence of each attribute, and every loaded URL has to match. That closes three variants of the same trick, including the one the previous fix introduced while closing it fortitle=.srcdocwas never inspected. Asrcdociframe inherits the embedding page's origin, so an entity encoded script inside it is same origin execution, and thescriptpattern missed it because the script was entity encoded.srcdocis now blocked outright and has no allowlist path.An inline
styleneeds no script to do harm. A fixed, full viewport<div>with a phishing link covers the real page, on a site with a membership payment flow. A blanket block was not an option: 18 distinct inline styles size the logo rows on the PyDay pages, and a theme rule overrides theheightattribute, so converting them is not faithful. Instead the attribute is a finding unless its value is onlywidth,heightor amargin, with a non negative length capped at four digits.position,z-index,background,url(), a CSS comment, a negative number and a viewport unit all fail. All 18 real values pass.Also added:
data:andvbscript:alongsidejavascript:, with thedata:pattern requiring a media type and a comma so that the word "data:" in prose is not a finding.The allowlist is empty
The tree had one entry, a Google Calendar iframe on the PyDay 2019 page, and that file was not in
CODEOWNERS, which is what made the two iframe attacks above reachable. The calendar now goes through the existinggoogle-embedshortcode, which validates the host, so the entry is gone and there is no exception to maintain or to abuse.Verified
The four payloads fail the check. 248 content files scanned, none skipped.
check-html-safetypasses on the real tree with no allowlist and no suppressions.check-contentreports 0 errors and 8 warnings. Build with zero warnings, 317 HTML files. The 2019 calendar still renders, now with a title, lazy loading and a referrer policy it did not have before.Beyond the four, these were tried and are all caught: an SVG with a script, entity encoded schemes including
jand
, a dangerous value that only appears after YAML decodes the front matter,<svg><animate attributeName="href">,<table background="javascript:">,srcsetwith adata:URL,<audio onerror>,<noscript><iframe>, an attribute split across lines,<meta http-equiv="refresh">,<base href>, and a script inside a markdown shortcode.Two defects found while checking that work, fixed here: a scheme finding reported the line the body starts on rather than its own line, because the match is found in text that has had its newlines stripped; and an accepted
stylelength was unbounded, sowidth:99999pxpassed the grammar and pushed the page off the viewport.What this does not do
The check scans
content/only. That is deliberate:content/is what an outside contributor can change, andlayouts/,themes/,static/,bin/andconfig.tomlare inCODEOWNERS. I verified there is nodata/ori18n/directory and that the theme reads neither, socontent/really is the only unowned, contributor editable, rendered path.But that model is not enforced today.
editionhas no required status checks, so this check does not block a merge, andrequire_code_owner_reviewsis off, so theCODEOWNERSfile this pull request adds has no effect at all. Both are organization settings. Until they are on, this is a tripwire that reports rather than a gate that stops.I will do the merge myself, in order.