Skip to content

Conversation

@mcbodge
Copy link
Owner

@mcbodge mcbodge commented Jan 9, 2026

No description provided.

- Introduced SECURITY.md to outline security measures and reporting for vulnerabilities.
- Updated README.md to reference SECURITY.md and summarize pipeline security considerations.
- Enhanced azure-pipelines.yaml with security notes regarding deployment protections.
- Improved path validation tests in PathValidation.Tests.ps1 for additional allowed paths.
- Updated PathValidation.ps1 to restrict deployment paths to standard IIS directories.
Copy link
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

This pull request adds comprehensive security documentation and enhances pipeline security measures to protect against unauthorized deployments from fork PRs and other attack vectors. The changes introduce a multi-layered security approach with path redaction, deployment stage protection, and detailed threat modeling.

Key Changes:

  • New SECURITY.md document with threat model, security controls, and incident response procedures
  • New LoggingUtility.ps1 module providing path and secret redaction for all deployment logs
  • Enhanced deployment stage conditions with kill switch, PR build blocking, and branch protection
  • Expanded PathValidation allowed roots to support additional IIS configurations

Reviewed changes

Copilot reviewed 18 out of 18 changed files in this pull request and generated 13 comments.

Show a summary per file
File Description
SECURITY.md New comprehensive security policy with threat model, controls, and incident response procedures
eng/scripts/Common/LoggingUtility.ps1 New logging utility module with path/secret redaction for secure CI/CD logging
eng/scripts/Common/LoggingUtility.Tests.ps1 Pester test suite for logging utility redaction functions
eng/scripts/Common/PathValidation.ps1 Expanded allowed deployment roots (D:\inetpub, C:\wwwroot, D:\wwwroot) with security documentation
eng/scripts/Common/PathValidation.Tests.ps1 Additional test coverage for new allowed root paths
eng/azure-pipelines.yaml Enhanced deployment conditions with kill switch, PR blocking, and comprehensive security comments
eng/README.md Expanded security section with pipeline security model, checklist, and configuration guidance
eng/scripts/*.ps1 (8 files) Updated all deployment scripts to use LoggingUtility for redacted output
.github/CODEOWNERS Expanded code ownership to include SECURITY.md and additional critical paths
.vscode/settings.json VSCode terminal auto-approve settings for Pester commands

Comment on lines 211 to 217
condition: |
and(
succeeded(),
eq(variables['deployEnabled'], 'true'),
ne(variables['Build.Reason'], 'PullRequest'),
eq(variables['Build.SourceBranch'], 'refs/heads/develop')
)
Copy link

Copilot AI Jan 9, 2026

Choose a reason for hiding this comment

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

The formatting of the multi-line condition uses a YAML literal block scalar (|) which preserves newlines and indentation. However, Azure Pipelines may interpret this differently than intended. The condition should evaluate as a single expression, but the newlines might cause parsing issues.

Consider using a folded scalar (>) instead, which folds newlines into spaces:

condition: >
  and(
    succeeded(),
    eq(variables['deployEnabled'], 'true'),
    ne(variables['Build.Reason'], 'PullRequest'),
    eq(variables['Build.SourceBranch'], 'refs/heads/develop')
  )

Or use a single-line format to avoid any ambiguity.

Copilot uses AI. Check for mistakes.
Comment on lines +124 to +132
```yaml
condition: |
and(
succeeded(), # Build must pass
eq(variables['deployEnabled'], 'true'), # Kill switch enabled
ne(variables['Build.Reason'], 'PullRequest'), # Not a PR build
eq(variables['Build.SourceBranch'], 'refs/heads/X') # Protected branch
)
```
Copy link

Copilot AI Jan 9, 2026

Choose a reason for hiding this comment

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

The formatting of the multi-line condition in the code example uses a YAML literal block scalar (|) which may not work correctly in Azure Pipelines conditions. Conditions are expressions, not multi-line strings, and this format could cause parsing errors.

The documentation should show the correct format. Either use folded scalar (>), or document that this is pseudocode and show the actual working format used in the pipeline.

Copilot uses AI. Check for mistakes.
$message = 'Artifact at D:\a\1\artifacts\file.zip'
$result = Get-RedactedMessage -Message $message
$result | Should -Be 'Artifact at [PIPELINE]\artifacts\file.zip'
}
Copy link

Copilot AI Jan 9, 2026

Choose a reason for hiding this comment

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

The path 'D:\a\1\artifacts\file.zip' would be redacted to '[PIPELINE]\artifacts\file.zip' based on the $env:PIPELINE_WORKSPACE = 'D:\a\1' setting. However, the redaction logic adds a trailing backslash to the environment variable path (line 76 of LoggingUtility.ps1: $pipelinePath = $env:PIPELINE_WORKSPACE.TrimEnd('\') + '\'), so it would look for 'D:\a\1\' in the message.

The test message 'Artifact at D:\a\1\artifacts\file.zip' contains 'D:\a\1\' as a substring, so the replacement would work. However, if someone writes 'Artifact at D:\a\1artifacts\file.zip' (missing the slash), it wouldn't be redacted. This is probably fine, but consider adding a comment or test case for this edge case.

Suggested change
}
}
It 'Does not redact PIPELINE_WORKSPACE when path lacks trailing backslash' {
$env:PIPELINE_WORKSPACE = 'D:\a\1'
$message = 'Artifact at D:\a\1artifacts\file.zip'
$result = Get-RedactedMessage -Message $message
$result | Should -Be $message
}

Copilot uses AI. Check for mistakes.
Comment on lines 95 to 96
# Only match paths that haven't been redacted yet (don't start with [)
$redacted = $redacted -replace '(?<!\[)([A-Z]):\\(?:[^\\]+\\)+', '[PATH]\'
Copy link

Copilot AI Jan 9, 2026

Choose a reason for hiding this comment

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

The regex pattern on line 96 uses a negative lookbehind (?<!\[) to avoid matching already-redacted paths. However, this pattern may incorrectly redact paths that appear after legitimate text containing a closing bracket. For example, "File not found] C:\Users\admin\file.txt" would not be redacted because the closing bracket precedes it.

Consider either:

  1. Making the pattern more specific to only avoid matching your redaction tokens (e.g., (?<!\[WEBROOT\]|IIS|PATH|PIPELINE|ARTIFACTS|SOURCE|TEMP))
  2. Or documenting this edge case behavior if it's intentional
Suggested change
# Only match paths that haven't been redacted yet (don't start with [)
$redacted = $redacted -replace '(?<!\[)([A-Z]):\\(?:[^\\]+\\)+', '[PATH]\'
# Only match paths that haven't been redacted yet (don't follow our redaction tokens)
$redacted = $redacted -replace '(?<!\[(?:WEBROOT|IIS|PATH|PIPELINE|ARTIFACTS|SOURCE|TEMP)\\)([A-Z]):\\(?:[^\\]+\\)+', '[PATH]\'

Copilot uses AI. Check for mistakes.
Comment on lines 68 to 96
# IMPORTANT: Apply static IIS path redactions FIRST (before generic fallback)
# This ensures specific patterns like [WEBROOT] are preserved
foreach ($path in $script:PathRedactions.Keys) {
$redacted = $redacted -replace [regex]::Escape($path), $script:PathRedactions[$path]
}

# Redact CI/CD pipeline paths (dynamic, from environment variables)
if ($env:PIPELINE_WORKSPACE) {
$pipelinePath = $env:PIPELINE_WORKSPACE.TrimEnd('\') + '\'
$redacted = $redacted -replace [regex]::Escape($pipelinePath), '[PIPELINE]\'
}
if ($env:BUILD_ARTIFACTSTAGINGDIRECTORY) {
$artifactPath = $env:BUILD_ARTIFACTSTAGINGDIRECTORY.TrimEnd('\') + '\'
$redacted = $redacted -replace [regex]::Escape($artifactPath), '[ARTIFACTS]\'
}
if ($env:BUILD_SOURCESDIRECTORY) {
$sourcePath = $env:BUILD_SOURCESDIRECTORY.TrimEnd('\') + '\'
$redacted = $redacted -replace [regex]::Escape($sourcePath), '[SOURCE]\'
}
if ($env:AGENT_TEMPDIRECTORY) {
$tempPath = $env:AGENT_TEMPDIRECTORY.TrimEnd('\') + '\'
$redacted = $redacted -replace [regex]::Escape($tempPath), '[TEMP]\'
}

# LAST: Redact any remaining absolute paths that look like server paths
# This catches paths not in our known list but still potentially sensitive
# Pattern: Drive letter followed by backslash and path segments (but NOT already redacted)
# Only match paths that haven't been redacted yet (don't start with [)
$redacted = $redacted -replace '(?<!\[)([A-Z]):\\(?:[^\\]+\\)+', '[PATH]\'
Copy link

Copilot AI Jan 9, 2026

Choose a reason for hiding this comment

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

The path redaction logic applies replacements sequentially, which could lead to double-redaction or incorrect output. For example, if C:\inetpub\wwwroot\app is processed:

  1. First, C:\inetpub\wwwroot\[WEBROOT]\ gives [WEBROOT]\app
  2. Then the generic pattern on line 96 won't match because of the [ lookbehind
    This appears to work correctly.

However, if environment variables overlap with static paths (e.g., $env:PIPELINE_WORKSPACE = 'C:\inetpub\temp'), the order of replacement could produce unexpected results. Consider documenting the replacement order and any potential edge cases, or ensure environment variable paths and static paths don't overlap.

Copilot uses AI. Check for mistakes.
}
} else {
Write-Warning "web.config not found at: $webConfigPath"
Write-WarnLog "web.config not found"
Copy link

Copilot AI Jan 9, 2026

Choose a reason for hiding this comment

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

The comment on line 76 says "web.config not found" but doesn't include the path information that was present in the original message. While this aligns with the security goal of not exposing paths, it makes debugging more difficult since operators won't know which path was checked.

Consider using the redacted path: Write-WarnLog "web.config not found at: $webConfigPath" which would output something like "web.config not found at: [WEBROOT]\MyApp\web.config", providing useful context while maintaining security.

Suggested change
Write-WarnLog "web.config not found"
Write-WarnLog "web.config not found at: $webConfigPath"

Copilot uses AI. Check for mistakes.
Comment on lines +5 to +6
"Import-Module": true,
".": true
Copy link

Copilot AI Jan 9, 2026

Choose a reason for hiding this comment

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

The VSCode settings for auto-approving terminal commands includes . (dot) which auto-approves dot-sourcing of any PowerShell script. This is potentially dangerous as it would automatically execute dot-sourced scripts without user review.

Consider either:

  1. Removing the . entry to require manual approval for dot-sourcing, or
  2. Being more specific about which scripts can be auto-approved (e.g., ".\eng\scripts\Common\*.ps1")

This is a developer convenience setting, so the risk is moderate (only affects local development), but it's worth considering given the security focus of this PR.

Suggested change
"Import-Module": true,
".": true
"Import-Module": true

Copilot uses AI. Check for mistakes.
eng/README.md Outdated
Comment on lines 239 to 246
condition: |
and(
succeeded(), # Build must pass
eq(variables['deployEnabled'], 'true'), # Kill switch enabled
ne(variables['Build.Reason'], 'PullRequest'), # Not a PR build
eq(variables['Build.SourceBranch'], 'refs/heads/X') # Protected branch only
)
```
Copy link

Copilot AI Jan 9, 2026

Choose a reason for hiding this comment

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

The documentation shows the multi-line condition format with a YAML literal block scalar (|), but as noted in previous comments, this format may not work correctly in Azure Pipelines. The condition expressions should use folded scalar (>) or be on a single line.

Update this example to match the recommended format to avoid confusion for users who copy this pattern.

Copilot uses AI. Check for mistakes.
Comment on lines +180 to +193
Describe 'Write-InfoLog' {
It 'Outputs redacted message via Write-Host' {
# We can't easily capture Write-Host output in Pester, but we can verify it doesn't throw
{ Write-InfoLog -Message 'Deploying to C:\inetpub\wwwroot\MyApp' } | Should -Not -Throw
}
}

Describe 'Write-WarnLog' {
It 'Outputs redacted warning' {
# Capture warning stream
$warnings = Write-WarnLog -Message 'Path not found: C:\inetpub\wwwroot\MyApp' 3>&1
$warnings | Should -Match '\[WEBROOT\]'
}
}
Copy link

Copilot AI Jan 9, 2026

Choose a reason for hiding this comment

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

The test suite for LoggingUtility.ps1 is missing test coverage for Write-ErrorLog. While Write-InfoLog and Write-WarnLog are tested, the error logging function should also be tested to ensure it properly redacts paths in error messages.

Consider adding a test case similar to the Write-WarnLog test that verifies error messages are properly redacted.

Copilot uses AI. Check for mistakes.
$Content = Get-RedactedMessage -Message $Content

# Redact potential secrets/tokens (simple patterns)
$Content = $Content -replace '(?i)(password|secret|key|token|bearer|authorization)["\s:=]+[^\s",}]+', '$1=[REDACTED]'
Copy link

Copilot AI Jan 9, 2026

Choose a reason for hiding this comment

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

The regex pattern for secret redaction on line 240 may not correctly handle all JSON/XML formats. The pattern (?i)(password|secret|key|token|bearer|authorization)["\s:=]+[^\s",}]+ would fail to redact values in formats like:

  • "password":"value" (no space after colon)
  • Multi-word values in quotes: "password": "my secret value" (stops at first space due to [^\s",}]+)
  • Values in XML: <password>value</password> (no quotes/colons)

Consider using a more robust pattern or parsing structured formats properly. For security-critical redaction, consider being more aggressive (e.g., redacting the entire value portion regardless of format).

Copilot uses AI. Check for mistakes.
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.

2 participants