security: PR body file handling, output sanitization, YAML adversarial tests, supply-chain attestations#3329
security: PR body file handling, output sanitization, YAML adversarial tests, supply-chain attestations#3329cotti wants to merge 4 commits into
Conversation
📝 WalkthroughWalkthroughThis PR hardens the docs-builder supply chain and CI output security across multiple dimensions. It introduces a new 🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches✨ Simplify code
Tip 💬 Introducing Slack Agent: The best way for teams to turn conversations into code.Slack Agent is built on CodeRabbit's deep understanding of your code, so your team can collaborate across the entire SDLC without losing context.
Built for teams:
One agent for your entire SDLC. Right inside Slack. Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In
`@tests/Elastic.Documentation.Configuration.Tests/ReleaseNotes/ReleaseNotesSerializationTests.cs`:
- Line 88: The InlineData entry in ReleaseNotesSerializationTests uses the
literal sequence "\\u202E right-to-left override" instead of the actual U+202E
bidi character; update the InlineData value in the
ReleaseNotesSerializationTests test (the InlineData attribute) to use the real
control character by replacing "\\u202E right-to-left override" with "\u202E
right-to-left override" (i.e., a single backslash so the C# string contains the
U+202E character) to match the other adversarial test cases.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Enterprise
Run ID: 367ba6de-47ec-4010-a765-c96aec58f86d
📒 Files selected for processing (10)
.github/actions/check-shai-hulud/action.yml.github/workflows/ci.yml.github/workflows/prerelease.yml.github/workflows/release.ymlsrc/services/Elastic.Changelog/Evaluation/ChangelogArtifactEvaluationService.cssrc/services/Elastic.Changelog/Evaluation/ChangelogPrEvaluationService.cssrc/services/Elastic.Changelog/Utilities/OutputSanitizer.cssrc/tooling/docs-builder/Commands/ChangelogCommand.cstests/Elastic.Changelog.Tests/Utilities/OutputSanitizerTests.cstests/Elastic.Documentation.Configuration.Tests/ReleaseNotes/ReleaseNotesSerializationTests.cs
| [InlineData("Title with > folded marker")] | ||
| [InlineData("Title with newline\nthen colon: injected: true")] | ||
| [InlineData("title:\nmalicious: true")] | ||
| [InlineData("\\u202E right-to-left override")] |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Verify escaped unicode literals are not accidentally used where actual control chars are intended.
rg -nP '\[InlineData\(".*\\\\u[0-9A-Fa-f]{4}' tests/Elastic.Documentation.Configuration.Tests/ReleaseNotes/ReleaseNotesSerializationTests.csRepository: elastic/docs-builder
Length of output: 115
🏁 Script executed:
cd tests/Elastic.Documentation.Configuration.Tests/ReleaseNotes && wc -l ReleaseNotesSerializationTests.csRepository: elastic/docs-builder
Length of output: 102
🏁 Script executed:
sed -n '70,100p' tests/Elastic.Documentation.Configuration.Tests/ReleaseNotes/ReleaseNotesSerializationTests.csRepository: elastic/docs-builder
Length of output: 1112
🏁 Script executed:
sed -n '74,152p' tests/Elastic.Documentation.Configuration.Tests/ReleaseNotes/ReleaseNotesSerializationTests.csRepository: elastic/docs-builder
Length of output: 2753
Use the actual U+202E character to match the pattern of other adversarial test cases.
Line 88 uses "\\u202E..." (escaped sequence producing the literal string \u202E), while other test cases like "Title ending with backslash \\" and "Title with newline\nthen colon: ..." use actual control characters. To properly exercise bidi-control handling in the serialization round-trip, this should use the actual U+202E character instead.
Proposed fix
-[InlineData("\\u202E right-to-left override")]
+[InlineData("\u202E right-to-left override")]📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| [InlineData("\\u202E right-to-left override")] | |
| [InlineData("\u202E right-to-left override")] |
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In
`@tests/Elastic.Documentation.Configuration.Tests/ReleaseNotes/ReleaseNotesSerializationTests.cs`
at line 88, The InlineData entry in ReleaseNotesSerializationTests uses the
literal sequence "\\u202E right-to-left override" instead of the actual U+202E
bidi character; update the InlineData value in the
ReleaseNotesSerializationTests test (the InlineData attribute) to use the real
control character by replacing "\\u202E right-to-left override" with "\u202E
right-to-left override" (i.e., a single backslash so the C# string contains the
U+202E character) to match the other adversarial test cases.
1 participant
Summary
P0 security remediation for
docs-builder, addressing four items fromelastic/docs-eng-team#532.
Each commit is self-contained and can be reviewed independently.
Issues addressed
ChangelogCommand.EvaluatePrnow reads PR body content fromPR_BODY_FILE(a path underRUNNER_TEMP) when set, falling back toPR_BODYwhen not. Caps reads at 256 KiB withStream.ReadExactlyAsync+ArrayPool<byte>. AOT-friendlyLoggerMessagesource generators for warning paths.Elastic.Changelog.Utilities.OutputSanitizer(defense-in-depth) strips C0/DEL control chars and enforces per-field length caps. Applied to every PR-derived output inChangelogPrEvaluationService.SetOutputs()and every artifact-derived output inChangelogArtifactEvaluationService.SetOutputs(). 19 unit tests covering null/empty inputs, control-char stripping, length truncation, and a GITHUB_OUTPUT delimiter-injection attempt.ReleaseNotesSerializationalready usesWithQuotingNecessaryStrings()+DisableAliases()+ a defensive title-quoting pass with full\uXXXXescaping. Added 12[Theory]cases + 2[Fact]cases covering quotes, colons, hash/anchor/alias markers, pipes, folded scalars, embedded newlines+colons, RTL overrides, and multi-line titles that try to injectimpact:/action:siblings. All assert exact round-trip equality and that no neighbouring field is polluted..github/actions/check-shai-huludcomposite extracted fromci.yml's inline check.prerelease.ymlandrelease.ymlcontainers job now: restore npm deps explicitly → run shai-hulud check before docker login → publish → resolve manifest digest per image (docker buildx imagetools inspect) → mint a signedactions/attest-build-provenance@v4attestation for each ofdocs-builder,docs-builder-mcp,docs-builder-api. Addsid-token: write+attestations: writepermissions.Commits
cf47ea20Get the PR Body as a file and safely handle it8daa6e50Add sanitization for input50054b40Add Serialization round-trip testsde75f67aAdd shai-hulud checksVerified
./build.sh(version + clean + lint + AOT compile) passes.ReleaseNotesSerializationTestspass (12 new theory cases + 3 pre-existing + 2 misc).OutputSanitizerTestspass.Coordination
After this PR merges, the next push to
mainwill triggerprerelease.yml, which publishes a new:edgedigest and itsattestation. Only after that push lands should the companion
elastic/docs-actionssecurity/verify-edge-attestationPR be merged— that PR adds
gh attestation verifyto the consumer side, whichwould fail until at least one signed
:edgeexists.Test plan
dotnet test tests/Elastic.Changelog.Tests— sanitizer tests pass.dotnet test tests/Elastic.Documentation.Configuration.Tests— YAML round-trip tests pass../build.shAOT compile passes.prerelease.ymlrun; confirm:- shai-hulud step passes,
- three
Attest <image>steps succeed,-
gh attestation verify oci://ghcr.io/elastic/docs-builder@<digest> -R elastic/docs-builder(run locally) verifies the new edge digest.Made with Cursor