build(deps): bump github/codeql-action from 3 to 4#3
Closed
dependabot[bot] wants to merge 1 commit into
Closed
Conversation
Bumps [github/codeql-action](https://github.com/github/codeql-action) from 3 to 4. - [Release notes](https://github.com/github/codeql-action/releases) - [Changelog](https://github.com/github/codeql-action/blob/main/CHANGELOG.md) - [Commits](github/codeql-action@v3...v4) --- updated-dependencies: - dependency-name: github/codeql-action dependency-version: '4' dependency-type: direct:production update-type: version-update:semver-major ... Signed-off-by: dependabot[bot] <support@github.com>
Contributor
Author
LabelsThe following labels could not be found: Please fix the above issues or remove invalid values from |
Contributor
Author
|
Looks like github/codeql-action is up-to-date now, so this is no longer needed. |
dependabot
Bot
deleted the
dependabot/github_actions/github/codeql-action-4
branch
2 tasks
emeraldleaf
added a commit
that referenced
this pull request
May 24, 2026
#25) * fix(service-defaults): security trio — middleware order, JWT, error trace ID Three small but distinct security hardening fixes in ServiceDefaults (addresses architecture-review findings #3, #5, #6). 1. MIDDLEWARE ORDER — CorrelationIdMiddleware after UseAuthentication The middleware reads ClaimTypes.NameIdentifier from context.User to populate the UserId logger scope key. Pre-auth, context.User is empty, so UserId was silently always null for every authenticated request — defeating half the context-propagation pipeline. UserId never made it into log scopes from the HTTP entry point and never reached Wolverine envelope headers via the ActivityBaggage → OutgoingContextMiddleware path. Reordered: UseExceptionHandler → UseAuthentication → UseAuthorization → CorrelationIdMiddleware. ExceptionHandler stays first so it still wraps everything below it. 2. JWT — explicit ValidateIssuerSigningKey + tightened ClockSkew ValidateIssuerSigningKey was implicit (JWT Bearer's default validates against JWKS-discovered keys). Made explicit so it's auditable + prevents a future config change from silently disabling it. ClockSkew defaulted to 5 MINUTES. Revoked/expired tokens were accepted for 5 extra minutes — material on typical 15-minute access-token lifetimes. Set to 30 seconds: covers reasonable inter-server clock drift, doesn't give attackers a long replay window. 3. EXCEPTION HANDLER — TraceId only, not full Activity.Id Activity.Current?.Id returns the full W3C traceparent ("00-<trace>-<span>-<flags>") and we were embedding that in every ProblemDetails response. The span ID is information about server-side handler call structure that clients have no business seeing. Switched to Activity.Current?.TraceId.ToString() — same correlation utility, no leak of internal call graph. None of the three needed any consumer changes; all are internal to ServiceDefaults. Build clean (the one CS0436 warning is the pre-existing benchmarks Program-conflict; unaffected). Co-Authored-By: Claude Opus 4.7 <noreply@anthropic.com> * fix(service-defaults): place CorrelationIdMiddleware between Auth and Authz CodeRabbit (PR #25) flagged that the previous fix moved the middleware too far down — to AFTER UseAuthorization — which works for populating UserId but drops the audit trail for 401/403 unauthorized-attempt logs. Correct placement is BETWEEN UseAuthentication and UseAuthorization: app.UseExceptionHandler(); app.UseAuthentication(); // populates context.User app.UseMiddleware<CorrelationIdMiddleware>(); // reads User, opens scope app.UseAuthorization(); // any 401/403 here has UserId in scope Two reasons: 1. CorrelationIdMiddleware needs context.User populated to read NameIdentifier (the original ordering bug — pre-auth, User was empty and UserId was always null). 2. Placing it BEFORE Authorization means the UserId scope is active during the authorization decision. If a user is rejected, the log line says "user X tried Y and was denied" — the audit trail we need. My previous fix dropped this by placing the middleware after Authorization, which would only attribute requests that actually passed authorization. Build clean. Single-line move of one app.UseMiddleware call. Co-Authored-By: Claude Opus 4.7 <noreply@anthropic.com> * test(service-defaults): unit tests for JWT options + GlobalExceptionHandler Covers the production-code changes in this PR so Codecov's patch-coverage gate stops complaining (was 8.33%; the prior fixes were uncovered because ServiceDefaults had no dedicated tests). GlobalExceptionHandlerTests (3): - TryHandleAsync_WhenActivityIsActive_ReturnsTraceIdOnly_NotFullActivityId: pins the security-critical change (Activity.Id → TraceId.ToString()). Asserts the response traceId equals activity.TraceId.ToString(), NOT activity.Id (which is the full W3C traceparent including span ID). Also asserts no hyphens — distinguishes the bare trace from the full "00-trace-span-flags" format. - TryHandleAsync_WhenNoActivity_FallsBackToHttpContextTraceIdentifier: covers the null-coalesce path. - TryHandleAsync_AlwaysReturnsProblemDetailsWithGenericDetail_NeverExceptionMessage: defense against information disclosure (CLAUDE.md "Error Handling"). Asserts the raw exception message doesn't appear in the response body even when the message contains sensitive-looking content like SQL. ServiceDefaultsJwtTests (3): - AddServiceDefaults_WhenAuthorityConfigured_SetsExplicitSigningKeyValidation - AddServiceDefaults_WhenAuthorityConfigured_SetsTightClockSkew - AddServiceDefaults_WhenAuthorityConfigured_RetainsCoreValidations All three boot a real Host.CreateApplicationBuilder + AddServiceDefaults with a configured authority, resolve the IOptionsMonitor<JwtBearerOptions>, and assert the TokenValidationParameters. Pins both the new hardening (ClockSkew=30s, ValidateIssuerSigningKey=true) AND the pre-existing core validations (Audience/Issuer/Lifetime) so a future refactor can't drop either silently. All 6 tests pass in 0.7s. Build clean (the one CS0436 warning is the pre-existing benchmarks Program-conflict; unaffected). Test placement follows the existing convention — ServiceDefaults tests live in service-test projects (OrderService.Tests.Unit/Application/ already has ContextPropagationMiddlewareTests, CorrelationIdMiddlewareTests, OutgoingContextMiddlewareTests). Not ideal long-term (duplication risk across the 4 service test projects), but matches the established pattern. The middleware-ordering change in Extensions.cs is harder to unit-test — it requires booting WebApplicationFactory and asserting the order of middleware execution at request time. Filed as a project-board issue for later; the JWT and ExceptionHandler tests cover the highest-risk lines of this PR (security hardening that could silently regress). Co-Authored-By: Claude Opus 4.7 <noreply@anthropic.com> --------- Co-authored-by: Claude Opus 4.7 <noreply@anthropic.com>
emeraldleaf
added a commit
that referenced
this pull request
May 27, 2026
… follow-up
Three new rules + one rule tightening + one open-issue acknowledgement,
all sourced from two Milan Jovanović / Kerim Kara articles on scaling
patterns. PR scope continues to be "encode the rules before the next
instance hits review."
Rules added:
1. NON-SARGABLE PREDICATES DEFEAT INDEXES — fix at write time.
A Where(...) that wraps a column in a function (u.Email.ToLower() == x,
o.CreatedAt.Date == today, EF.Functions.ILike with leading wildcard)
defeats any B-tree index on that column. Fix at write time: normalize
on insert/update + Where against the normalized column, or use
case-insensitive column collation. Leading-wildcard substring search
isn't B-tree-indexable in any database — escalate to Postgres tsvector
full-text or a dedicated search engine when load justifies it.
CatalogService/Features/SearchProducts.cs already documents the
leading-wildcard trade-off explicitly; updated its cross-reference to
point at the new rule by name.
2. FAN-OUT BELONGS ON THE MESSAGE BUS, NOT IN A SYNCHRONOUS HANDLER LOOP.
A handler that iterates a recipient list inline and awaits a sender
per recipient holds the request open for N × per-recipient-latency
and concentrates work on one process. Right shape: publish one
Wolverine message per recipient (or per batch of K), return immediately,
let per-recipient handlers run in parallel under
MaxDegreeOfParallelism throttle. Preventative — not retroactively
violated today (NotificationService receives one event = one outbound
notification).
3. PARALLELIZE INDEPENDENT AWAITS WITH TASK.WHENALL.
Sequential await of independent I/O calls serializes latency for free.
When a handler makes N independent I/O calls (N gRPC to different
services, N HTTP to different external APIs, N queries against
DIFFERENT DbContexts), Task.WhenAll pays the max latency, not the sum.
Reference: OrderService/Features/PlaceOrder.cs:93 (gRPC fan-out over
per-line GetProductAsync with the DbContext-safety caveat documented
inline). Explicit caveats encoded: don't WhenAll dependent operations,
operations sharing the same DbContext scope (not thread-safe), or
operations whose per-call failure observability matters (WhenAll
surfaces only the first exception).
Rule tightening (from prior architecture-reviewer feedback):
4. 202 ACCEPTED RULE — two definitional clarifications.
(a) "Tracking row" can be the aggregate itself when its ID is the
polling key (POST /api/v1/orders pattern), not necessarily a
separate jobs table.
(b) "Commit atomically" branches by dispatch path: bus.InvokeAsync
gets AutoApplyTransactions for free; inline persist+publish needs
the explicit BeginTransactionAsync → SaveChangesAsync → CommitAsync
wrap from the existing Outbox-outside-handler rule. Cross-reference
added.
STATUS.md follow-up:
5. POST /api/v1/payments/process noted as a partial Should-consider
under the new long-running-work rule. Endpoint returns 202 but the
handler awaits Stripe synchronously; typical sub-second but tail
latency on degraded states is seconds-to-30s. Deferred (current
shape works for the demo; rule is encoded preventatively).
Note on the second Kerim Kara article ("Every Senior .NET Developer Has
Defended This Architecture"): most of its lessons are already encoded in
NextAurora — the I*Repository deletion (PR #30), the CatalogService
Clean→VSA collapse (PR #31), the "Interfaces earn their keep through
consumer substitution" rule, the integration-tests-over-mocked-repository
discipline, the DbContext-direct data-access shape. The article reads
as a victory lap for the simplicity refactor. The one net-new takeaway
was the sequential-awaits trap, encoded here as rule #3.
Co-Authored-By: Claude Opus 4.7 <noreply@anthropic.com>
emeraldleaf
added a commit
that referenced
this pull request
May 27, 2026
…#39) * chore(claude): encode Factory Pattern / keyed-services rule across config Adds the "factory pattern earns its keep at 2+ impls" rule to the three canonical encoding sites + a worked sketch at the reference site: - CLAUDE.md: new bullet right after the consumer-substitution rule. Codifies that .NET keyed services (AddKeyedScoped + [FromKeyedServices]) is the canonical multi-impl factory; don't hand-roll IPortFactory; don't pre-build the factory while there's only one impl. NotificationService cited as the canonical "ready for the factory, not yet wearing it" case. - .claude/agents/architecture-reviewer.md: new section keyed to Infrastructure/DependencyInjection.cs files. Two flagged patterns: premature factory (Must-fix) and missing factory at 2+ impls (Should-consider). Explicit guidance that IServiceProvider's keyed- services API IS the canonical factory. - .coderabbit.yaml: matching path_instructions block under **/Infrastructure/DependencyInjection.cs, encoding the same two patterns so CodeRabbit catches future violations at PR-review time. - NotificationService/Infrastructure/DependencyInjection.cs: composition- root XML doc now sketches the future keyed-services rewrite that the current single-AddScoped will become when SendGrid/Twilio actually ship — so the rule lands with a concrete reference shape in code, not just in config. Why now: PR #30 + #31 collapsed the I*Repository wrappers and tightened the consumer-substitution rule. The factory-pattern shape is the natural next codification — same lesson (layers without capability are speculative coupling), one tier up in abstraction. Co-Authored-By: Claude Opus 4.7 <noreply@anthropic.com> * docs(readme): drop stale Clean Architecture refs + trim verbose preamble PR #31 collapsed CatalogService from Clean Architecture to VSA, but the README still referenced CatalogService as a "deliberate Clean Architecture carve-out" and described the design as "mixed per-service architecture." Both claims contradicted the (correct) Project Structure section that said VSA across all five services. Changes: - Collapsed the four-bullet "About this repo" preamble into two bullets: Monorepo + single architectural shape (was three contradictory bullets plus a long MySQL-translation aside), and the two-database-engines bullet retained. - Updated the How It Works doc description: "Clean Architecture" -> "VSA layout" (matches docs/how-it-works.md which was already updated in the collapse). - Dropped the seven `Source: file.excalidraw` subtitle lines under the hero diagram + each of the six reference diagrams. The SVG is already embedded and clickable to full-size; the editable .excalidraw sources are now noted once in the section intro. Net: -20 lines, -1 contradiction. Co-Authored-By: Claude Opus 4.7 <noreply@anthropic.com> * chore(claude): apply review fixes + encode async-API / 202 Accepted rule Two threads in one commit: REVIEW FIXES (3) from the architecture-reviewer pass on the prior commits: 1. Reconciled categorization drift on "MISSING FACTORY at 2+ impls" between architecture-reviewer.md (was "Must-fix on absence when per-call selection is required") and .coderabbit.yaml (was "Should-consider", no qualifier). Both now agree: Must-fix when per-call routing is intended (latent bug — plain AddScoped registrations collide silently, DI returns last-registered impl every time, dropping routing intent); Should-consider when impls are interchangeable (no live bug, but tighten for explicitness). 2. Split architecture-reviewer.md's dual-category bullet into two cleaner bullets — one for the per-call-selection Must-fix case, one for the interchangeable Should-consider case. 3. Narrowed the Infrastructure DI section heading: the glob now stands alone (`**/Infrastructure/DependencyInjection.cs`), and the broader port-adapter scope is lifted into the body so the glob and the prose claim match. NEW RULE — long-running work / 202 Accepted shape. Encoded across CLAUDE.md "Performance Rules" + .coderabbit.yaml Endpoints path + architecture-reviewer Endpoints section. Rule: if a write handler can take more than ~1 second, reshape as 202 Accepted (validate + persist intent + publish Wolverine message + return job ID with Location header). Background handler does the work; client polls or gets pushed. NextAurora already has all the machinery (Wolverine + Service Bus + outbox + saga) — the rule is "use it when a handler would otherwise block on minutes-scale work." Reference shape: POST /api/v1/orders (place → publish OrderPlaced → return OrderId; downstream PaymentService + ShippingService handle saga via async events). Same rule applies to Wolverine handlers themselves — if the handler body runs for minutes, factor the work into a follow-up message handler. Why now: the rule isn't actively violated in the codebase today (no minutes-scale synchronous endpoints exist), but the encoding is what guarantees future endpoints don't introduce one. Same logic as the factory pattern rule landed in the prior commit — encode the shape before the violation, not after. Verified no existing "See CLAUDE.md" markers paraphrase a contradictory version of the new rule (the two nearest markers reference money calculations and consumer substitution, both unaffected). Co-Authored-By: Claude Opus 4.7 <noreply@anthropic.com> * chore(claude): encode 3 more rules + tighten 202 Accepted + STATUS.md follow-up Three new rules + one rule tightening + one open-issue acknowledgement, all sourced from two Milan Jovanović / Kerim Kara articles on scaling patterns. PR scope continues to be "encode the rules before the next instance hits review." Rules added: 1. NON-SARGABLE PREDICATES DEFEAT INDEXES — fix at write time. A Where(...) that wraps a column in a function (u.Email.ToLower() == x, o.CreatedAt.Date == today, EF.Functions.ILike with leading wildcard) defeats any B-tree index on that column. Fix at write time: normalize on insert/update + Where against the normalized column, or use case-insensitive column collation. Leading-wildcard substring search isn't B-tree-indexable in any database — escalate to Postgres tsvector full-text or a dedicated search engine when load justifies it. CatalogService/Features/SearchProducts.cs already documents the leading-wildcard trade-off explicitly; updated its cross-reference to point at the new rule by name. 2. FAN-OUT BELONGS ON THE MESSAGE BUS, NOT IN A SYNCHRONOUS HANDLER LOOP. A handler that iterates a recipient list inline and awaits a sender per recipient holds the request open for N × per-recipient-latency and concentrates work on one process. Right shape: publish one Wolverine message per recipient (or per batch of K), return immediately, let per-recipient handlers run in parallel under MaxDegreeOfParallelism throttle. Preventative — not retroactively violated today (NotificationService receives one event = one outbound notification). 3. PARALLELIZE INDEPENDENT AWAITS WITH TASK.WHENALL. Sequential await of independent I/O calls serializes latency for free. When a handler makes N independent I/O calls (N gRPC to different services, N HTTP to different external APIs, N queries against DIFFERENT DbContexts), Task.WhenAll pays the max latency, not the sum. Reference: OrderService/Features/PlaceOrder.cs:93 (gRPC fan-out over per-line GetProductAsync with the DbContext-safety caveat documented inline). Explicit caveats encoded: don't WhenAll dependent operations, operations sharing the same DbContext scope (not thread-safe), or operations whose per-call failure observability matters (WhenAll surfaces only the first exception). Rule tightening (from prior architecture-reviewer feedback): 4. 202 ACCEPTED RULE — two definitional clarifications. (a) "Tracking row" can be the aggregate itself when its ID is the polling key (POST /api/v1/orders pattern), not necessarily a separate jobs table. (b) "Commit atomically" branches by dispatch path: bus.InvokeAsync gets AutoApplyTransactions for free; inline persist+publish needs the explicit BeginTransactionAsync → SaveChangesAsync → CommitAsync wrap from the existing Outbox-outside-handler rule. Cross-reference added. STATUS.md follow-up: 5. POST /api/v1/payments/process noted as a partial Should-consider under the new long-running-work rule. Endpoint returns 202 but the handler awaits Stripe synchronously; typical sub-second but tail latency on degraded states is seconds-to-30s. Deferred (current shape works for the demo; rule is encoded preventatively). Note on the second Kerim Kara article ("Every Senior .NET Developer Has Defended This Architecture"): most of its lessons are already encoded in NextAurora — the I*Repository deletion (PR #30), the CatalogService Clean→VSA collapse (PR #31), the "Interfaces earn their keep through consumer substitution" rule, the integration-tests-over-mocked-repository discipline, the DbContext-direct data-access shape. The article reads as a victory lap for the simplicity refactor. The one net-new takeaway was the sequential-awaits trap, encoded here as rule #3. Co-Authored-By: Claude Opus 4.7 <noreply@anthropic.com> * chore(claude): encode Server-side pricing rule + tighten paraphrase Surfaced by /check-rules audit: `OrderService/Features/PlaceOrder.cs:138` had a `See CLAUDE.md.` marker with no specific section named, paraphrasing a principle ("never trust client-submitted prices for money calculations") that wasn't a named rule in CLAUDE.md. Option 2 (per the audit prompt): add the named rule, tighten the paraphrase to point at it. Changes: - CLAUDE.md "Security Requirements" gets a new named bullet: "Server-controlled fields are computed server-side, never trusted from the client" — covers money fields (Price/Currency/Tax), authorization identifiers (BuyerId/SellerId), state-machine columns (Status), and security flags (IsAdmin/IsDeleted). Specifies the failure mode (price tampering — client submits Price=0.01 for a $999 product) and the canonical pattern (fetch authoritative value from its source; treat the request DTO as untrusted input). References PlaceOrder.cs as the reference example. Cross-links to the existing "Mass assignment" check in the architecture-reviewer. - OrderService/Features/PlaceOrder.cs:138 cross-reference tightened from generic `See CLAUDE.md.` to the named rule. No other files paraphrase this principle today (verified via the prior /check-rules audit run that found this single drift). Co-Authored-By: Claude Opus 4.7 <noreply@anthropic.com> --------- Co-authored-by: Claude Opus 4.7 <noreply@anthropic.com>
emeraldleaf
added a commit
that referenced
this pull request
May 31, 2026
…st didn't suppress findings ## What happened Previous attempt (b955474) added `.gitleaks.toml` with a `[[allowlists]]` block scoping path + regex to the three factory files. CI showed the config loaded successfully ("using gitleaks config from GITLEAKS_CONFIG env var: .gitleaks.toml") and the [extend] block worked, but the allowlist's path+regex match did NOT fire against the `generic-api-key` findings in gitleaks 8.24.3. Three findings surfaced: - PaymentApiFactory.cs:95 (the original literal) - ShippingApiFactory.cs:82 (the original literal) - CLAUDE.md:219 (because the previous commit quoted the high-entropy literal verbatim in the new rule's prose — scanner walks docs too) ## The fix Use the proven mechanism: inline `// gitleaks:allow` markers at the end of the offending line. Reliable across gitleaks 8.x. Suppresses only the specific finding line — pasting a real key into the same file still trips because the marker only covers THAT line. Changes: 1. OrderApiFactory.cs / PaymentApiFactory.cs / ShippingApiFactory.cs — added `// gitleaks:allow` to the end of the connection-string line that carries the fake `SharedAccessKey`. Updated the explanatory comments above each to reflect that the inline marker is the suppressor, not the .gitleaks.toml [[allowlists]] block. 2. CLAUDE.md — removed the high-entropy base64 literal from the rule's prose (this is what triggered finding #3 on the last run). The rule now describes the convention without reproducing the trigger substring. Added explicit guidance: do NOT reproduce the literal in CLAUDE.md or any other prose — the scanner walks every diff. Also updated the suppression mechanism description to name the inline marker as load-bearing. 3. .gitleaks.toml — kept as supplementary documentation of the convention but updated the header comment + allowlist description to be honest about the limitation. The block stays for convention-encoding; the inline markers do the actual suppression. Broadened the regex from the exact literal to `SharedAccessKey=[A-Za-z0-9+/=]{20,}` so the documented allowlist would cover any base64-shaped fake SharedAccessKey in those three files if/when the gitleaks semantics allow it to fire. ## What I'd debug next if this still fails If the inline markers don't suppress either (very unlikely — the mechanism is the standard documented escape hatch), the fallback is to make the fake string non-base64-shaped (e.g. lowercase letters only, no `=` padding) which drops below gitleaks' entropy threshold for generic-api-key. Cost: the fake no longer "looks real" so the factories' realism docstring takes a hit. Not preferred. Co-Authored-By: Claude Opus 4.7 <noreply@anthropic.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
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.
Bumps github/codeql-action from 3 to 4.
Release notes
Sourced from github/codeql-action's releases.
... (truncated)
Changelog
Sourced from github/codeql-action's changelog.
... (truncated)
Commits
fbba1e0Rebuild933238eUpdate changelog and version after v4.35.3e46ed2cMerge pull request #3867 from github/update-v4.35.3-8c6e48dbeb73d1d1Add changelog entry for #385324e0bb0Reorder changelog entriesec298daUpdate changelog for v4.35.38c6e48dMerge pull request #3865 from github/update-bundle/codeql-bundle-v2.25.37190983Add changelog note2bb2095Update default bundle to codeql-bundle-v2.25.3Dependabot will resolve any conflicts with this PR as long as you don't alter it yourself. You can also trigger a rebase manually by commenting
@dependabot rebase.Dependabot commands and options
You can trigger Dependabot actions by commenting on this PR:
@dependabot rebasewill rebase this PR@dependabot recreatewill recreate this PR, overwriting any edits that have been made to it@dependabot show <dependency name> ignore conditionswill show all of the ignore conditions of the specified dependency@dependabot ignore this major versionwill close this PR and stop Dependabot creating any more for this major version (unless you reopen the PR or upgrade to it yourself)@dependabot ignore this minor versionwill close this PR and stop Dependabot creating any more for this minor version (unless you reopen the PR or upgrade to it yourself)@dependabot ignore this dependencywill close this PR and stop Dependabot creating any more for this dependency (unless you reopen the PR or upgrade to it yourself)