Skip to content

Commit 7100932

Browse files
πŸ“– [Docs]: Fill gaps in coding standards (#53)
Adds only the guidance that was missing after comparing the candidate standards from PSModule/docs PR #61 against the existing canonical MSXOrg/docs coding standards. ## Topic-by-topic comparison ### GitHub Actions Already covered in `src/docs/Coding-Standards/GitHub-Actions.md`: - SHA pinning and updating pinned actions - explicit least-privilege `permissions` - OIDC instead of long-lived cloud secrets - `vars` vs `secrets` - untrusted context interpolation via `env:` - action vs reusable workflow boundaries - PowerShell-first workflow scripting - script extraction into composite actions - reusable workflows with colocated composite actions - concurrency, named jobs/steps, logging, summaries, PR comments, and required checks Missing and added: - `actions/checkout` should use `persist-credentials: false` unless a job intentionally pushes - `pull_request_target` and `workflow_run` should be avoided unless the elevated context is required, and privileged work must not execute untrusted code - actionlint/zizmor severity handling and audit focus areas ### PowerShell Already covered in `src/docs/Coding-Standards/PowerShell/`: - advanced functions, classes, scripts - `#Requires -Modules` and version constraints - Verb-Noun naming, casing, full command and parameter names - OTBS, indentation, spacing, line length, splatting - null comparisons, string matching, reuse order, .NET performance paths - output streams, ShouldProcess, credentials, and error behavior Missing and added: - predictable data-module and integration-module verb mapping - lowercase PowerShell language keywords - avoiding semicolons, ternary expressions, and regions in shared code - built-in checks for blank strings and wildcard detection - avoiding explicit `-Verbose` / `-Debug` forwarding to internal calls - cross-platform environment reads through `[Environment]::GetEnvironmentVariable()` when the provider is not the point ## Validation - `pwsh .github/scripts/Update-DocumentationIndex.ps1 -Check` - `pwsh .github/scripts/Test-DocumentationLink.ps1`
1 parent db8d017 commit 7100932

3 files changed

Lines changed: 25 additions & 0 deletions

File tree

β€Žsrc/docs/Coding-Standards/GitHub-Actions.mdβ€Ž

Lines changed: 19 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -81,6 +81,7 @@ this section is the Actions-specific view of it.
8181
level, since the workflow and its one job are equivalent.)
8282
- **Declare permissions per job**, not only at workflow level, and grant only
8383
the scopes that job needs. Most jobs need no more than `contents: read`.
84+
- **Disable checkout's persisted credentials unless the job intentionally pushes.** `actions/checkout` writes the token into the local Git configuration by default; set `persist-credentials: false` for read-only jobs so later steps cannot accidentally push with it. When a job must push, isolate that work in the smallest possible job, grant the write scope there, and make the persisted credential an explicit review point.
8485

8586
```yaml
8687
# Default-deny floor at the top; each job opts into exactly what it needs.
@@ -145,6 +146,14 @@ directly into a `run:` script allows shell injection. **Pass untrusted values
145146
through an `env:` variable** and reference the variable, which is not re-parsed
146147
as script.
147148

149+
Avoid `pull_request_target` and `workflow_run` unless the workflow genuinely
150+
needs the elevated context. Both can connect untrusted contributions to tokens,
151+
secrets, or artifacts from a more privileged run. If one is required, split the
152+
workflow so untrusted code is never executed with a writable token: validate or
153+
build the contributor's code in the unprivileged workflow, then let the
154+
privileged workflow consume only trusted, explicitly produced artifacts or
155+
metadata.
156+
148157
```yaml
149158
# Correct β€” value arrives as an environment variable, not inlined into the shell
150159
- name: Print the PR title
@@ -217,6 +226,8 @@ jobs:
217226
# Sequential, state-sharing work belongs in one job as ordered steps.
218227
- name: Check out the repository
219228
uses: actions/checkout@de0fac2e4500dabe0009e67214ff5f5447ce83dd # v6.0.2
229+
with:
230+
persist-credentials: false
220231
221232
- name: Build
222233
shell: pwsh
@@ -658,6 +669,14 @@ Both run through [super-linter](https://github.com/super-linter/super-linter) on
658669
every push and pull request, so a workflow that breaks this standard fails the
659670
build.
660671

672+
Treat `actionlint` errors and high-severity `zizmor` findings as blockers.
673+
Medium-severity `zizmor` findings are fixed unless the workflow documents why
674+
the risk is accepted; low-severity findings are fixed when the simpler, safer
675+
shape is available. In practice, an audit starts by checking for unpinned
676+
actions, broad or inherited secrets, missing `permissions`, persisted checkout
677+
credentials, untrusted context interpolation, and elevated triggers such as
678+
`pull_request_target` or `workflow_run`.
679+
661680
```text
662681
# Single-file action β€” main.<ext> at the action root
663682
.github/actions/link-check/

β€Žsrc/docs/Coding-Standards/PowerShell/Functions.mdβ€Ž

Lines changed: 1 addition & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -79,6 +79,7 @@ Send each kind of message to the stream built for it, so a caller can capture, r
7979
- **Results** are objects on the output stream β€” emit them implicitly by naming the object on its own line; do **not** use `return $obj` to emit, and in a pipeline function emit from `process`, not `end`.
8080
- **Emit one object type**, matching `[OutputType()]`.
8181
- **`Write-Verbose`** for status a caller may want (`-Verbose`), **`Write-Debug`** for maintainer breadcrumbs (`-Debug`), and **`Write-Progress`** for progress that need not persist.
82+
- **Do not pass `-Verbose` or `-Debug` explicitly to internal calls** just because the current function was invoked with them. Common parameters are caller controls; write to the stream in the current function and let the caller decide which streams to show or capture.
8283
- **`Write-Warning`** and **`Write-Error`** for warnings and non-terminating errors.
8384
- **`Write-Host`** only for `Show-` or `Format-` verbs or an interactive prompt β€” never for data another command might consume.
8485

β€Žsrc/docs/Coding-Standards/PowerShell/index.mdβ€Ž

Lines changed: 5 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -38,6 +38,7 @@ These hold for all PowerShell, whatever the construct:
3838
- **`PascalCase`** for functions, parameters, public variables, and class members; `camelCase` for local variables.
3939
- **Full cmdlet names, never aliases** (`Where-Object`, not `?`; `ForEach-Object`, not `%`).
4040
- **Full parameter names, and standard ones.** Pass parameters by name and avoid positional arguments in shared code β€” `Get-Process -Name pwsh`, not `Get-Process pwsh` β€” so a call survives parameter-set changes and reads clearly. Name your own parameters after PowerShell's built-ins (`Path`, `Name`, `ComputerName`), not `$Param_Computer`.
41+
- **Map data and integration verbs predictably.** Data modules use `ConvertFrom-` / `ConvertTo-` around a neutral PowerShell object shape, `Import-` / `Export-` for file or store round trips, and `Format-`, `Merge-`, `Compare-`, `Test-`, or `Remove-<Noun>Entry` for structure operations. API and integration modules name commands after the resource and intent, not the transport: map `GET` to `Get-`, create-style `POST` to `New-` / `Add-`, `PUT` / `PATCH` to `Set-` / `Update-`, `DELETE` to `Remove-`, and non-CRUD operations to the approved verb that matches the user intent.
4142
- **Set `$ErrorActionPreference = 'Stop'`** at the top of every script and module so errors are terminating, not silently swallowed.
4243
- **Emit objects, not formatted text.** Return rich objects and let the caller format; reserve `Write-Host` for genuine console UX, and use `Write-Verbose` / `Write-Information` for progress narration.
4344

@@ -49,8 +50,10 @@ These rules define the layout; [PSScriptAnalyzer](#toolchain) enforces them (its
4950
- **Indent with four spaces, never tabs**, and indent comment-based help to align with the function it documents.
5051
- **One space around operators and after commas** (`$a -eq $b`, `@(1, 2, 3)`), and **one space between a type and the name** β€” `[string] $Name`, not `[string]$Name`.
5152
- **`elseif` is one word**, not `else if`.
53+
- **Use lowercase language keywords** (`if`, `else`, `foreach`, `class`, `enum`, `return`) and reserve PascalCase for commands and named symbols.
5254
- **Blank lines separate logical blocks.** No trailing whitespace, and end every file with a single newline.
5355
- **Keep code lines readable β€” aim for roughly 120 columns.** When a call grows long, prefer [splatting](#idioms-and-pitfalls) over backtick line-continuations.
56+
- **Avoid semicolons, ternary expressions, and regions in shared code.** Put one statement on each line, use ordinary `if` / `else` when a conditional value matters, and structure code with functions, modules, and headings rather than `#region` blocks.
5457

5558
## Idioms and pitfalls
5659

@@ -60,13 +63,15 @@ Beyond the basics, these language-specific habits keep PowerShell correct and fa
6063
- **Splat calls that carry many parameters.** Build a `@{}` of parameters and splat it (`Get-Thing @params`) instead of a long line of `-Param value` pairs or backtick continuations; it reads better and diffs cleanly.
6164
- **Put `$null` on the left of a comparison** β€” `$null -eq $x`, never `$x -eq $null`. Against a collection the right-hand form *filters* rather than tests. Use `-contains` / `-in` for membership, never `-eq`.
6265
- **Match text with the operator built for it.** Use `-like` for wildcard patterns and `-match` for regular expressions instead of hand-rolled string surgery; both default to case-insensitive, so add the `-c` prefix (`-clike`, `-cmatch`, `-ceq`) when a comparison must be case-sensitive.
66+
- **Use the built-in intent checks for strings and wildcards.** Use `[string]::IsNullOrWhiteSpace($value)` for blank input and `[System.Management.Automation.WildcardPattern]::ContainsWildcardCharacters($pattern)` when deciding whether a value contains wildcard syntax.
6367
- **Reuse before you build.** Work down the [reuse order](../Functions.md#reuse-before-you-build) β€” a built-in cmdlet or operator, then an existing function (public or private), then a trusted module (`#Requires -Modules` / `RequiredModules`), then your own code (small logic inline, a larger capability as its own module). State a dependency's acceptable versions as a [version range](Version-Constraints.md).
6468
- **PowerShell already *is* .NET; work at that level rather than wrapping it.** Casts, type accelerators (`[datetime]`, `[int]`), the `-split` / `-replace` / `-match` operators, and member methods (`.Trim()`, `.Where()`) all resolve to the base class library β€” using .NET means reaching for BCL types and methods for the computation, not restating everything as `[Namespace.Type]::Method(...)`. Where idiomatic PowerShell already resolves to the same .NET call, leave it; reach for explicit .NET only where it is measurably faster or more precise, and keep cmdlets and the pipeline where you need them for glue or readability.
6569
- **Do the work in .NET when you implement it.** When you write the logic yourself β€” or fix an internal function that is too slow or imprecise on a hot path β€” call the .NET base class library directly instead of a cmdlet pipeline: `[System.IO.File]::ReadAllText($path)` over `Get-Content -Raw`, `[System.IO.Path]::Combine(...)` for paths, `[System.Text.StringBuilder]` for repeated concatenation, `[int]::TryParse(...)` for parsing. .NET methods are faster and their contracts are precise; keep cmdlets where their clarity is worth more than the speed. The next two rules are specific cases.
6670
- **Suppress unwanted output with `$null = ...`** (or `[void]` for method calls), not `| Out-Null` β€” the pipeline form is markedly slower on hot paths.
6771
- **Build collections with a typed list, not `+=` in a loop.** `$a += $x` reallocates the whole array every iteration; use `[System.Collections.Generic.List[T]]` with `.Add()`, and prefer a cmdlet's `-Filter` over piping to `Where-Object` on large sets.
6872
- **Guard a value that must not change.** Declare it with `Set-Variable -Name Pi -Value 3.14159 -Option ReadOnly` β€” or `-Option Constant` for one that can never be reassigned or removed β€” so an accidental write fails loudly instead of quietly winning.
6973
- **Keep secrets out of source, and never `Invoke-Expression` untrusted input.** Accept credentials as a `[PSCredential]` parameter with the `[Credential()]` attribute rather than calling `Get-Credential` inside a reusable function, so a caller can pass one they already hold, and take other sensitive values as `[securestring]`. Guard state-changing commands with `ShouldProcess` (see [Functions](Functions.md)); the wider rules live in the [Security](../Security.md) baseline.
74+
- **Read cross-platform environment values through .NET when the provider is not the point.** Prefer `[Environment]::GetEnvironmentVariable('NAME')` for process environment reads that should behave the same on every platform; use `$env:NAME` when you are intentionally working through the PowerShell environment provider.
7075

7176
## Toolchain
7277

0 commit comments

Comments
Β (0)