Skip to content

docs: clarify frontmatter hash is stale-lock detection, not tamper protection#24198

Merged
pelikhan merged 1 commit intomainfrom
copilot/update-security-architecture
Apr 3, 2026
Merged

docs: clarify frontmatter hash is stale-lock detection, not tamper protection#24198
pelikhan merged 1 commit intomainfrom
copilot/update-security-architecture

Conversation

Copy link
Copy Markdown
Contributor

Copilot AI commented Apr 3, 2026

The frontmatter hash was described using "integrity" and "security" language implying tamper protection — which it cannot provide, since anyone with repo write access can modify both .md and .lock.yml together. Its actual purpose is detecting stale lock files (i.e. the workflow was edited without recompiling).

Changes

  • frontmatter-hash-specification.md

    • Renamed purpose item from "Security: Detect unauthorized modifications" → "Stale lock detection: Identify when the lock file is out of sync with the source workflow"
    • Rewrote Security Considerations to explicitly state the hash does not guarantee tamper protection and explain why
  • check_workflow_timestamp_api.cjs

    • Module docstring: reframed from "integrity validation" to "stale lock file detection"; added explicit note that tamper protection is out of scope
    • core.info log and warning messages: replaced "integrity check failed" with "could not verify whether lock file is up to date"

@pelikhan
Copy link
Copy Markdown
Collaborator

pelikhan commented Apr 3, 2026

@lpcox fyi

@pelikhan pelikhan marked this pull request as ready for review April 3, 2026 01:05
Copilot AI review requested due to automatic review settings April 3, 2026 01:05
@pelikhan pelikhan merged commit 7122e61 into main Apr 3, 2026
51 checks passed
@pelikhan pelikhan deleted the copilot/update-security-architecture branch April 3, 2026 01:05
Copy link
Copy Markdown
Contributor

Copilot AI left a comment

Choose a reason for hiding this comment

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

Pull request overview

Clarifies that the frontmatter hash is intended to detect stale lock files (lock out of sync with source) rather than providing tamper protection/security guarantees.

Changes:

  • Updates the frontmatter hash specification to describe the feature as stale-lock detection and explicitly disclaim tamper protection.
  • Rewords the workflow lock check script’s docstring and user-facing logs/warnings away from “integrity/security” language toward “stale/unverifiable” language.

Reviewed changes

Copilot reviewed 2 out of 2 changed files in this pull request and generated 3 comments.

File Description
docs/src/content/docs/reference/frontmatter-hash-specification.md Reframes purpose + security considerations to emphasize stale-lock detection and clarify lack of tamper protection.
actions/setup/js/check_workflow_timestamp_api.cjs Updates module documentation and warning/log messaging to align with stale-lock detection framing.

💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.

Comment on lines 195 to 199
if (!hashComparison) {
// Could not compute hash - be conservative and fail
core.warning("Could not compare frontmatter hashes - assuming lock file is outdated");
const warningMessage = `Lock file '${lockFilePath}' integrity check failed! Could not verify frontmatter hash for '${workflowMdPath}'. Run 'gh aw compile' to regenerate the lock file.`;
const warningMessage = `Lock file '${lockFilePath}' is outdated or unverifiable! Could not verify frontmatter hash for '${workflowMdPath}'. Run 'gh aw compile' to regenerate the lock file.`;

Copy link

Copilot AI Apr 3, 2026

Choose a reason for hiding this comment

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

The failure/warning message text here has changed (no longer includes "integrity check failed"), but the unit tests still assert on the old substring (e.g., actions/setup/js/check_workflow_timestamp_api.test.cjs lines ~237, ~248, ~350, etc.). Please update those expectations to match the new wording, otherwise CI will fail.

See below for a potential fix:

    const warningMessage = `Lock file '${lockFilePath}' is outdated or unverifiable! Frontmatter integrity check failed for '${workflowMdPath}'. Run 'gh aw compile' to regenerate the lock file.`;

    let summary = core.summary
      .addRaw("### ⚠️ Workflow Lock File Warning\n\n")
      .addRaw("**WARNING**: Could not verify whether lock file is up to date. Frontmatter integrity check failed.\n\n")

Copilot uses AI. Check for mistakes.

The frontmatter hash provides:
1. **Change detection**: Verify that workflow configuration has not changed between compilation and execution
1. **Stale lock detection**: Identify when the compiled lock file is out of sync with the source workflow (e.g. after editing the `.md` file without recompiling)
Copy link

Copilot AI Apr 3, 2026

Choose a reason for hiding this comment

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

This purpose statement implies that any edit to the .md workflow file will be detected as a stale lock, but the spec explicitly excludes markdown body content from the hash (see the Excluded Fields list). Consider narrowing the wording to “after editing the workflow frontmatter (or imported frontmatter) without recompiling” to avoid overstating what the hash detects.

Suggested change
1. **Stale lock detection**: Identify when the compiled lock file is out of sync with the source workflow (e.g. after editing the `.md` file without recompiling)
1. **Stale lock detection**: Identify when the compiled lock file is out of sync with the source workflow (e.g. after editing the workflow frontmatter or imported frontmatter without recompiling)

Copilot uses AI. Check for mistakes.
Comment on lines +8 to +10
* the workflow was edited without recompiling the lock file. It does not
* provide tamper protection — use code review to guard against intentional
* modifications.
Copy link

Copilot AI Apr 3, 2026

Choose a reason for hiding this comment

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

The docstring says it detects cases where “the workflow was edited without recompiling the lock file,” but the comparison is based on the frontmatter hash (not the full markdown workflow content). To avoid implying broader coverage than implemented, consider rephrasing to “frontmatter (and imported frontmatter) was edited without recompiling.”

Suggested change
* the workflow was edited without recompiling the lock file. It does not
* provide tamper protection use code review to guard against intentional
* modifications.
* frontmatter (and imported frontmatter) was edited without recompiling the
* lock file. It does not provide tamper protection use code review to guard
* against intentional modifications.

Copilot uses AI. Check for mistakes.
lpcox added a commit that referenced this pull request Apr 3, 2026
Tests asserted 'integrity check failed' but the error message was
changed to 'is outdated or unverifiable' in #24198. Updated all 6
assertions to match the current production error format.

Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
lpcox added a commit that referenced this pull request Apr 3, 2026
…24221)

* fix: clear stale GH_HOST and remove false fork PR detection

Two bugs interacted to cause PR checkout failures on github.com repos:

1. configure_gh_for_ghe.sh: For github.com, the script returned early
   without clearing GH_HOST. A stale value (e.g., localhost:18443 from
   a prior DIFC proxy run) persisted, causing 'gh pr checkout' to fail
   with 'none of the git remotes correspond to GH_HOST'. Now explicitly
   unsets stale GH_HOST values on the github.com path.

2. pr_helpers.cjs: detectForkPR() checked head.repo.fork to detect fork
   PRs, but this flag indicates whether the *repository itself* is a fork
   (common in OSS), not whether the PR is cross-repo. This caused false
   positives that forced the slow 'gh pr checkout' path (which depends on
   GH_HOST) instead of fast 'git fetch' (which doesn't). Removed the
   fork flag check; now only uses full_name comparison.

Fixes #24208
Closes #24217
Closes #24218

Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>

* test: update push_to_pr_branch test for corrected fork detection

The test 'should detect fork PR via fork flag and fail early' assumed
that head.repo.fork=true with same full_name meant a cross-repo fork PR.
After fixing detectForkPR() to only use full_name comparison (#24208),
same-repo PRs in forked repositories are correctly treated as non-fork,
allowing push to proceed.

Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>

* test: update checkout_pr_branch tests for corrected fork detection

The mock detectForkPR() in checkout_pr_branch.test.cjs replicated the
old fork-flag-based logic, making tests inconsistent with production.
Updated the mock to match the corrected full_name-only comparison and
updated assertions to verify that same-repo PRs in forked repositories
use the fast git fetch path.

Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>

* test: fix check_workflow_timestamp_api tests for updated error messages

Tests asserted 'integrity check failed' but the error message was
changed to 'is outdated or unverifiable' in #24198. Updated all 6
assertions to match the current production error format.

Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>

---------

Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
lpcox added a commit that referenced this pull request Apr 3, 2026
Tests asserted 'integrity check failed' but the error message was
changed to 'is outdated or unverifiable' in #24198. Updated all 6
assertions to match the current production error format.

Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants