feat(templates): require signatures for javascript templates - #7514
Merged
ehsandeep merged 2 commits intoJul 3, 2026
Merged
Conversation
Contributor
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Organization UI Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (15)
👮 Files not reviewed due to content moderation or server errors (15)
🚥 Pre-merge checks | ✅ 3 | ❌ 2❌ Failed checks (2 warnings)
✅ Passed checks (3 passed)
✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 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 `@internal/tests/integration/javascript_test.go`:
- Around line 497-501: The signing logic in runJavascriptTemplateAndGetResults
only covers jsTestcases, so templates passed in through filePath (like
javascriptNoAccess) can still hit the trust gate unsigned. Update the signing
step to include the current filePath being executed, or otherwise ensure every
JavaScript fixture used by this helper is signed before execution. Use the
runJavascriptTemplateAndGetResults flow and the templates.SignTemplate call as
the place to fix this.
In `@internal/tests/integration/runner_test.go`:
- Around line 203-205: The signing predicate for JavaScript templates is too
narrow and only matches protocol fixtures, so workflow JavaScript subtemplates
can bypass the signing prelude. Update needsSignedJavascriptTemplates to mirror
the workflow-aware logic used by needsSignedCodeTemplates by including the
workflow JavaScript fixture path pattern alongside the existing
protocol/javascript prefix. Keep the change localized to
needsSignedJavascriptTemplates so the integration runner applies signing
consistently across both template families.
🪄 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: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 61a1dcc3-18f5-4502-aa21-a11779ec47b7
📒 Files selected for processing (30)
README.mdREADME_CN.mdREADME_ES.mdREADME_ID.mdREADME_JP.mdREADME_KR.mdREADME_PT-BR.mdREADME_TR.mdcmd/nuclei/main.gocmd/tmc/main.gointernal/runner/runner.gointernal/tests/integration/code_test.gointernal/tests/integration/javascript_krbroast_test.gointernal/tests/integration/javascript_test.gointernal/tests/integration/matcher-status_test.gointernal/tests/integration/runner_test.golib/config.gopkg/catalog/loader/loader.gopkg/catalog/loader/loader_test.gopkg/protocols/javascript/js_test.gopkg/templates/capability.gopkg/templates/compile.gopkg/templates/compile_test.gopkg/templates/parser_stats.gopkg/templates/stats.gopkg/templates/templates.gopkg/templates/templates_test.gopkg/templates/templates_utils.gopkg/templates/workflows.gopkg/types/types.go
Contributor
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 `@internal/tests/functional/functional_test.go`:
- Around line 390-407: The functional test helper currently treats only a bare
-javascript as present, so explicit bool assignments like -javascript=false are
ignored and then overwritten by currentFunctionalArgs. Update hasFunctionalFlag
to recognize assignment forms for the javascript flag (for example, any arg
starting with the flag name plus "=") and keep currentFunctionalArgs from
appending -javascript when that flag is already explicitly set. Add a regression
test in the functional test suite to cover -javascript=false and verify it
remains unchanged.
🪄 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: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: dd55b74d-ddbe-47b2-b994-a4bfeee3dea7
📒 Files selected for processing (1)
internal/tests/functional/functional_test.go
Contributor
There was a problem hiding this comment.
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
pkg/templates/signer/tmpl_signer.go (1)
97-106: 🗄️ Data Integrity & Integration | 🟠 Major | ⚡ Quick winImported file content isn't CRLF-normalized, leaving the cross-platform fix incomplete.
contentis normalized vianormalizeTemplateContentForSignaturebefore signing/verifying, but bytes read fromtmpl.GetFileImports()are appended raw (buff.Write(bin)). If an imported file has CRLF line endings (the exact Windows-checkout scenario this change targets), it will still produce different signatures depending on the checkout's line-ending state, defeating the purpose of the fix for templates with imports.🔧 Normalize imported file bytes too
for _, file := range tmpl.GetFileImports() { bin, err := os.ReadFile(file) if err != nil { return "", err } buff.WriteRune('\n') - buff.Write(bin) + buff.Write(normalizeTemplateContentForSignature(bin)) }(apply the analogous change in
Verifyas well)Also applies to: 149-158
🤖 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 `@pkg/templates/signer/tmpl_signer.go` around lines 97 - 106, The signing and verification paths in Sign and Verify still append imported file bytes from tmpl.GetFileImports() without normalization, so the CRLF fix is incomplete for templates with imports. Update both functions to run the bytes read from os.ReadFile through normalizeTemplateContentForSignature before writing them into the buffer, using the existing buff setup as the reference point, so imported files are treated the same as the main content.
🤖 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.
Outside diff comments:
In `@pkg/templates/signer/tmpl_signer.go`:
- Around line 97-106: The signing and verification paths in Sign and Verify
still append imported file bytes from tmpl.GetFileImports() without
normalization, so the CRLF fix is incomplete for templates with imports. Update
both functions to run the bytes read from os.ReadFile through
normalizeTemplateContentForSignature before writing them into the buffer, using
the existing buff setup as the reference point, so imported files are treated
the same as the main content.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 17a8ace2-0c7c-4dfa-801f-95f57634680f
📒 Files selected for processing (2)
pkg/templates/signer/tmpl_signer.gopkg/templates/signer/tmpl_signer_test.go
dwisiswant0
marked this pull request as draft
July 1, 2026 11:29
Unsigned javascript templates should not be accepted into execution paths when template signing is enforced. Apply the signature check to javascript protocol templates in both direct loading and workflows, and report those skips separately from code templates. Signed-off-by: Dwi Siswanto <git@dw1.io>
dwisiswant0
force-pushed
the
dwisiswant0/feat/templates/require-signatures-for-javascript-templates
branch
from
July 2, 2026 07:48
1e2d7fb to
f8c7f7b
Compare
The verifier already canonicalizes CRLF to LF before checking the digest, but the signer still hashed the raw file content. Templates signed from a CRLF checkout could fail verification later, which showed up on Windows after javascript fixtures started being signed in the integration harness. Use the same canonical content for signing and verification, and add a CRLF sign-and-verify regression. Signed-off-by: Dwi Siswanto <git@dw1.io>
dwisiswant0
marked this pull request as ready for review
July 2, 2026 12:14
ehsandeep
approved these changes
Jul 3, 2026
ehsandeep
deleted the
dwisiswant0/feat/templates/require-signatures-for-javascript-templates
branch
July 3, 2026 00:20
4 tasks
4 tasks
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.
Proposed changes
Unsigned javascript templates should not be
accepted into execution paths when template
signing is enforced.
Apply the signature check to javascript protocol
templates in both direct loading and workflows,
and report those skips separately from code
templates.
Closes #7513
Proof
Checklist
A detailed high-level summary could not be generated for this review. Here is an overview derived from the analyzed file changes: