Skip to content

[l10n/automation] Add comment renderer and manifest validator - #941

Open
Chaitanya-Keyal wants to merge 1 commit into
SeedSigner:devfrom
Chaitanya-Keyal:l10n/comment-tools
Open

[l10n/automation] Add comment renderer and manifest validator#941
Chaitanya-Keyal wants to merge 1 commit into
SeedSigner:devfrom
Chaitanya-Keyal:l10n/comment-tools

Conversation

@Chaitanya-Keyal

@Chaitanya-Keyal Chaitanya-Keyal commented Jun 22, 2026

Copy link
Copy Markdown
Contributor

Note

One of the l10n source-string automation PRs (#940, #941, #942, #943, #944). The manifest schema this validates against, and the tests' conftest, are added in #940.

Description

Problem or Issue being addressed

The review manifest is produced by untrusted PR code but rendered into a PR comment by a trusted job. We need to
(a) validate the manifest before trusting it and
(b) render it without letting attacker-controlled source strings break out of the comment.

Solution

Adds two tools:

  • validate_manifest.py -- validates a manifest against the schema and checks two trust anchors GitHub sets authoritatively (the repository, and the head SHA the triggering run was built from). This is the security boundary between the untrusted producer and the trusted consumer.
  • render_comment.py -- renders the advisory comment. Source strings are shown inside a ```diff``` fence with newlines flattened and a +/- prefix on every line, so no msgid can start a line and break the fence or inject markup. A test feeds a hostile msgid and proves no breakout.

This pull request is categorized as a:

  • Other (l10n automation)

Checklist

I ran pytest locally

  • All tests passed before submitting the PR

(python -m pytest l10n/automation/tests -- these tests live outside the project's default testpaths and are run explicitly.)


I included screenshots of any new or modified screens

  • N/A

I added or updated tests

  • Yes

I tested this PR hands-on on the following platform(s):

  • Github Actions

I have reviewed these notes:

  • Keep your changes limited in scope.
  • If you uncover other issues or improvements along the way, ideally submit those as a separate PR.
  • The more complicated the PR, the harder it is to review, test, and merge.
  • We appreciate your efforts, but we're a small team of volunteers so PR review can be a very slow process.
  • Please only "@" mention a contributor if their input is truly needed to enable further progress.
  • I understand

Comment thread l10n/automation/render_comment.py Outdated
Comment thread l10n/automation/validate_manifest.py Outdated
Comment thread l10n/automation/render_comment.py Outdated
render_comment.py renders the advisory comment from a validated manifest;
untrusted source strings are contained in a diff code fence, with a test
proving fence-breakout is not possible. validate_manifest.py checks a
manifest against the schema and the repository / head-SHA trust anchors
before the trusted comment job acts on it. Includes tests.

These tests rely on the diff engine's conftest for import-path setup, so
merge after the diff engine.

@wolgwang1729 wolgwang1729 left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

Status: Summer of Bitcoin

Development

Successfully merging this pull request may close these issues.

2 participants