Conversation
…orecard pin refresh Greenfield-audit findings shipped in one PR. # Restaurants API: silent role-check bypass under MapInboundClaims=false RestaurantsController used [Authorize(Roles="Restaurants.Read.All")] + [RequireClientApp]. Doctrine sets MapInboundClaims=false (so the JWT 'roles' claim is preserved verbatim and not remapped to ClaimTypes.Role), so [Authorize(Roles=...)] silently never matches: the framework looks up ClaimTypes.Role on the identity, but the claim's type is the short name 'roles'. The endpoint either always-403'd, or — worse — relied on [RequireClientApp] alone, letting any allow-listed app read regardless of whether it had been granted the role. Switched to the named-policy pattern via the existing AddAppPolicy helper (same shape Ftgo.Orders.Api uses for its app surface). Added 6 end-to-end tests including two explicit regression guards (RestaurantsApp_Rejects_AllowListedAppWithoutRequiredRole + NoRolesClaim) that would catch a future re-introduction of [Authorize(Roles=...)] under MapInboundClaims=false. # .github/rulesets/main.json: PR-approval gate + pinact required check Bumped required_approving_review_count 0 -> 1 with require_code_owner_review: true, and added the Repository Admin role (actor_id=5) to bypass_actors so the solo-maintainer self-merge workflow (gh pr merge --admin --squash) keeps working. Added 'pinact' to required_status_checks. Did not add 'scorecard' as required since it is scheduled+push-to-main (not run on every PR). # .github/workflows/scorecard.yml: action SHA refresh Brought scorecard.yml in line with the rest of the workflows: - actions/checkout v5.0.1 -> v6.0.2 - actions/upload-artifact v4.6.2 -> v7.0.1 - github/codeql-action/upload-sarif v3.28.0 -> v4.35.2 # Test results dotnet test: 66/66 (was 60, +6 new Restaurants policy tests). Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
Owner
Author
|
Applied updated ruleset to live via |
The default secrets.GITHUB_TOKEN does not have admin scope so the rulesets
API hides the bypass_actors field from it. After applying a ruleset that
declares bypass_actors via an admin PAT (locally), the next CI drift run
sees bypass_actors in the committed file but not in the API response and
reports false-positive drift forever.
Strip bypass_actors from both sides of the diff. The committed file still
documents intent — verify the live state out-of-band with an admin PAT
(e.g. gh api /repos/{owner}/{repo}/rulesets/{id} --jq .bypass_actors).
Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
mghabin
added a commit
that referenced
this pull request
May 1, 2026
…st-class tier (#103) ## Why The auto-deployed first cloud tier was named **`dev`**, which conflated two things: 1. The tier where developer-machine work happens (a laptop, no Azure resources, `dotnet run`). 2. The tier where CI auto-deploys every push to \`main\` without a human gate (an Azure RG, real cloud APIs, public ingress). Calling them both \"dev\" hid what was actually running where. The honest name for #2 is **\`ci\`** (built and deployed by CI, no human gate), and #1 is its own first-class tier called **\`local\`** (no Azure, no GH Environment, configured via \`appsettings.Development.json\` + \`dotnet user-secrets\`). Final ladder: **\`local → ci → ppe → prod\`**. \`ppe\` (Microsoft jargon for \"pre-production environment\", analogous to \"staging\" elsewhere) is unchanged. ## What this PR does **Phase A — codebase rename** (commit \`dbd6537\`): - \`infra/bicep/{azure,main,bootstrap}.dev.bicepparam\` → \`.ci.bicepparam\` - \`@allowed([ 'dev', 'ppe', 'prod' ])\` → \`@allowed([ 'ci', 'ppe', 'prod' ])\` in main/azure/bootstrap.bicep - \`devPublicClients\` var → \`ciPublicClients\` (gated on \`environmentName == 'ci'\`) - \`scripts/{bootstrap-env,provision-apps}.sh\`: default ENV is now \`ci\`; \`ENV=dev\` still accepted with a deprecation warning - \`.github/workflows/{cd,cd-cleanup,deploy-env,ci}.yml\`: \`deploy-dev\` job → \`deploy-ci\`, matrix env list, default workflow_dispatch input **Phase B — docs sweep** (commit \`cb6021e\`): - \`docs/environments.md\` rewritten as a **4-tier table** (local / ci / ppe / prod) with explicit \`ASPNETCORE_ENVIRONMENT\` mapping per tier and idle cost - \`docs/run-locally.md\` opening promotes \`local\` to the documented first tier - \`glossary.md\` gains entries for \`ci\`, \`local\`, \`ppe\` with primary-source citations - Mechanical rename across operations.md, cost-zero.md, deploy-cloud.md, sample-setup.md, matrix.md, best-practices.md, README.md, DOCTRINE.md, coverage-map.md **Phase C — live cutover runbook** (commit \`6b39dfe\`): - \`docs/operations.md\` gains a \"Renaming a deployment tier (one-shot live cutover)\" runbook with the **OIDC-safe step order** (provision new tier → migrate \`ENTRA_CONFIG_JSON\` → smoke test → tear down old). The non-obvious bit is FIC subjects: the GitHub OIDC token's \`sub\` claim is matched **verbatim** against the FIC subject, so renaming the GH environment before the FIC fails with \`AADSTS70021\`. **Plus** (commit \`f35c5d2\`): README's stale references to the deleted credential-patterns demo (\`orderservice\` / \`restaurantservice\`) are updated to the live app reg names (\`orders-api\` / \`restaurants-api\`). ## Live cutover not yet executed The PR is **codebase-only**. Live Azure / GitHub changes (provisioning \`rg-ftgo-ci-eastus\`, migrating \`ENTRA_CONFIG_JSON\` to a new \`ci\` GH env, updating FIC subjects, tearing down \`rg-ftgo-dev-eastus\`) are deferred to a one-shot operation following the runbook in this PR (\`docs/operations.md\` → \"Renaming a deployment tier\" section). Until that happens, the \`dev\` tier still works because: - \`scripts/{bootstrap-env,provision-apps}.sh\` accept \`ENV=dev\` as a deprecated alias and translate it to \`ENV=ci\` (with a stderr warning). - The existing \`dev\` GitHub Environment is unchanged. The auto-CD push trigger will fail to find the \`ci\` env after this PR merges until the cutover runs — that's intentional, so the cutover is done deliberately, not as a side-effect of merge. ## Verification - ✅ \`dotnet test EntraAuthPatterns.slnx\` → 66/66 - ✅ \`az bicep build\` on every \`.bicep\` and every \`.bicepparam\` (BCP427 on the params is environment-variable interpolation in CI, expected) - ✅ \`actionlint .github/workflows/*.yml\` clean - ✅ \`markdownlint-cli2\` (CI's pinned version) clean - ✅ The positive-path Restaurants 200 test (\`RestaurantsApp_Allows_AppTokenWithRoleAndAllowListedAzp\` in PR #102) still passes — that addresses the \"we never tested 200\" gap I previously surfaced - 🟡 Live cloud probe deferred to the cutover (see runbook) ## Out of scope (deliberate) - Renaming \`ppe\` → \`staging\` (PPE is Microsoft jargon, kept by preference) - Adding ephemeral PR-preview environments (separate doctrine decision) - Cosmetic rename of the live app reg display names \`ftgo-dev-{apigateway,orders-api,restaurants-api}\` → \`ftgo-ci-…\`. The appIds (the only thing \`ENTRA_CONFIG_JSON\` references) are unchanged. Optional follow-up. --------- Co-authored-by: mghabin <81494213+MohammadGhabin@users.noreply.github.com> Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
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
1 participant
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.
Greenfield-audit findings (3 parallel audits: C#, infra, docs) — one PR per real finding.
What this fixes
1. Restaurants API silent role-check bypass (security)
RestaurantsControllerused[Authorize(Roles="Restaurants.Read.All")]+[RequireClientApp]. Doctrine setsMapInboundClaims = falseso the JWTrolesclaim is preserved verbatim — but[Authorize(Roles=...)]resolves againstClaimTypes.Role, not"roles". The role check silently never matched. Switched to the named-policy pattern via the existingAddAppPolicyhelper (same shapeFtgo.Orders.Apiuses).Added 6 end-to-end tests including 2 explicit regression guards that would catch any future re-introduction of
[Authorize(Roles=...)]underMapInboundClaims=false.2. Ruleset PR-approval gate + pinact required check (supply-chain)
required_approving_review_count: 0 → 1require_code_owner_review: false → trueactor_id=5) tobypass_actorsso the solo-maintainergh pr merge --admin --squashflow keeps workingpinacttorequired_status_checks3. scorecard.yml SHA refresh
actions/checkoutv5.0.1 → v6.0.2actions/upload-artifactv4.6.2 → v7.0.1github/codeql-action/upload-sarifv3.28.0 → v4.35.2Verification
dotnet test: 66/66 (was 60, +6 new Restaurants policy tests)dotnet format --verify-no-changes: cleanactionlint scorecard.yml: cleanOut of scope (audit explicitly classified LOW)