fix(security): enforce disable_* alarm-type settings server-side (#236)#238
Merged
hokiepokedad2 merged 2 commits intomainfrom Apr 17, 2026
Merged
Conversation
![github-code-quality[bot]](https://avatars.githubusercontent.com/u/9919?s=60&v=4){kind=small}
Comment on lines
+131
to
+138
| foreach (var alarm in arr.EnumerateArray()) | ||
| { | ||
| if (alarmFilter(alarm)) | ||
| { | ||
| hasAny = true; | ||
| break; | ||
| } | ||
| } |
Contributor
Author
Independent ReviewReviewed as if seeing this for the first time, walking every code path that creates / updates / clean-toggles an alarm. Security correctness — cleanAll paths gated:
Architecture
Performance
Frontend
Test coverage~30 new tests: Backward compat
Findings
Recommendation: APPROVENo blockers. Security correctness is solid, architecture is clean, perf trade-offs are documented, tests are substantial, docs (CHANGELOG + CLAUDE.md |
hokiepokedad2
added a commit
that referenced
this pull request
Apr 17, 2026
…p.spec Three follow-ups from the iteration-4 review on PR #238: - TestAlertController now reuses `DisableFeatureKeys.ByTrackingType` and `IFeatureGate.EnsureEnabledAsync` instead of maintaining its own type→key dictionary and calling `ISiteSettingService.GetBoolAsync` directly. The throw is mapped to 403 by the existing global `FeatureDisabledExceptionFilter` so the wire contract is unchanged. Test was updated to match. - Disable-guard tests added to all 9 remaining alarm services (Raid/Egg/Quest/Invasion/Lure/Nest/Gym/MaxBattle/FortChange) so each one independently verifies that `CreateAsync` and `BulkCreateAsync` short-circuit at `EnsureEnabledAsync` without touching the proxy. Previously only `MonsterService` and `CleaningService` had explicit per-service tests; the pattern is identical across the 10 services but representative coverage left 8 service files silently regressable. - New `src/app/app.spec.ts` covers the `alarmNavItems` / `settingsNavItems` filter logic — including the iteration-1 fix that removed the admin bypass. Without this spec, "admin sees disabled nav items" could regress without breaking any existing test.
Contributor
Author
|
Closed all 3 review items in commit da9dddc:
Verification: backend 1031 passing, frontend 650 passing (was 1013 / 627), |
The disable_mons / disable_raids / disable_quests / disable_invasions /
disable_lures / disable_nests / disable_gyms / disable_maxbattles /
disable_fort_changes site settings only hid navigation items in the SPA
— every alarm controller was reachable via direct API calls regardless
of the toggle, and several service-to-service paths (quick-pick apply,
profile duplicate/import, cleaning toggle) wrote alarms past the
controller layer entirely.
Defense-in-depth gating across three layers:
- Constants & types: `DisableFeatureKeys` (single source of truth +
tracking-type→key map), `FeatureDisabledException` carrying the
violated key.
- Gate service: `IFeatureGate` exposes `IsEnabledAsync` (boolean check)
and `EnsureEnabledAsync` (throws). Logs at Info for an audit trail.
- Controller layer: `[RequireFeatureEnabled]` on all 10 alarm
controllers; inline check in `TestAlertController`. Global
`FeatureDisabledExceptionFilter` maps thrown exceptions to 403. Both
paths share `FeatureDisabledResponse.Create` so the wire format
can't drift.
- Service layer: every alarm service's `CreateAsync`,
`BulkCreateAsync`, and `UpdateAsync` calls `EnsureEnabledAsync`
first. `CleaningService.ToggleCleanAsync` and
`ProfileOverviewService.{DuplicateProfile,ImportAlarms}Async`
pre-validate before mutating state.
Frontend follow-through: `disabledFeatureGuard` on 9 alarm routes
(falls back to allowing navigation if `loadOnce` errors — backend still
enforces); 403 interceptor distinguishes feature-disabled from generic
permission denial via `disableKey` and redirects to `/dashboard`; left
nav hides disabled items for admins too; `ERROR.FEATURE_DISABLED` in
all 11 locales; empty alarm-group label and divider hidden.
Performance: `SiteSettingService.GetByKeyAsync` now wrapped in
`IMemoryCache` with 5-min TTL and explicit invalidation on writes —
otherwise the dashboard would add ~10 MySQL roundtrips per page load.
Operator note: the toggle now means "nobody uses this feature" —
including admins. To use a disabled feature, flip the toggle off in
Admin → Settings. `disable_eggs` doesn't exist; eggs share
`disable_raids` because they share the raid UI.
…p.spec Three follow-ups from the iteration-4 review on PR #238: - TestAlertController now reuses `DisableFeatureKeys.ByTrackingType` and `IFeatureGate.EnsureEnabledAsync` instead of maintaining its own type→key dictionary and calling `ISiteSettingService.GetBoolAsync` directly. The throw is mapped to 403 by the existing global `FeatureDisabledExceptionFilter` so the wire contract is unchanged. Test was updated to match. - Disable-guard tests added to all 9 remaining alarm services (Raid/Egg/Quest/Invasion/Lure/Nest/Gym/MaxBattle/FortChange) so each one independently verifies that `CreateAsync` and `BulkCreateAsync` short-circuit at `EnsureEnabledAsync` without touching the proxy. Previously only `MonsterService` and `CleaningService` had explicit per-service tests; the pattern is identical across the 10 services but representative coverage left 8 service files silently regressable. - New `src/app/app.spec.ts` covers the `alarmNavItems` / `settingsNavItems` filter logic — including the iteration-1 fix that removed the admin bypass. Without this spec, "admin sees disabled nav items" could regress without breaking any existing test.
hokiepokedad2
force-pushed
the
fix/236-server-side-disable-alarm-enforcement
branch
from
da9dddc to
021899b
Compare
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.
Summary
Closes #236.
disable_*alarm-type site settings only hid navigation in the SPA — every alarm controller was reachable via direct API calls regardless of the toggle, and several service-to-service paths (quick-pick apply, profile duplicate/import, cleaning toggle) wrote alarms past the controller layer entirely.This PR enforces the toggles server-side with defense-in-depth across three layers — controller filter, service-layer guard, and global exception handler — so no future caller can accidentally bypass the gate. Frontend also gets a route guard, a 403 interceptor refinement, and the admin no longer sees disabled nav items (the original bug was a UI/API mismatch — leaving one in place for admins would have recreated it in miniature).
What's in here
Backend gating
IFeatureGate(Core/Pgan.PoracleWebNet.Core.Abstractions/Services/IFeatureGate.cs) wrappingISiteSettingService.GetBoolAsyncwithIsEnabledAsync/EnsureEnabledAsync. The latter throwsFeatureDisabledException. Logs at Info for an audit trail.DisableFeatureKeys(Core/Pgan.PoracleWebNet.Core.Models/) — single source of truth for the 9 disable keys plus a tracking-type→key map used byProfileOverviewServiceandTestAlertController.[RequireFeatureEnabled]resource filter on all 10 alarm controllers.TestAlertControllerdoes the same check inline since the alarm type is a path parameter.EnsureEnabledAsynccalls in every alarm service'sCreateAsync,BulkCreateAsync, andUpdateAsync— closes the bypasses found by review (QuickPickService → MonsterService.CreateAsync, etc.).CleaningService.ToggleCleanAsyncandProfileOverviewService.{DuplicateProfile,ImportAlarms}Asyncpre-validate before any state mutation so a partial copy can't fail mid-loop.FeatureDisabledExceptionFiltermaps any service-thrown exception to HTTP 403; both response paths shareFeatureDisabledResponse.Create(disableKey)so the wire format can't drift.Frontend follow-through
disabledFeatureGuard(disableKey)factory applied to all 9 alarm routes. WrapsloadOnce()in try/catch — if settings load fails, allows navigation since the backend still enforces.error.error?.disableKeyand routes feature-disabled responses through a clearer toast + redirect to/dashboardinstead of leaving the user on a broken page.ERROR.FEATURE_DISABLEDtranslation added to all 11 locales.Performance
SiteSettingService.GetByKeyAsyncwrapped inIMemoryCache(5-min TTL, explicit invalidation on writes). Without this the dashboard would add ~10 MySQL roundtrips per page load (one gate check per alarm endpoint).Hygiene
disable_fort_changeswas missing fromSettingsMigrationService.BooleanKeysandCategoryMap(pre-existing oversight) — now seeded.Feature Gatingsubsection inCLAUDE.mddocumenting the pattern for future contributors.Behavioral changes admins should know
disable_eggssetting. Eggs share the raid disable toggle (they share the raid UI in the SPA).EggServiceandEggControlleruseDisableFeatureKeys.Raidswith explicit comments.Test plan
dotnet test— 1013 passed (added ~30 tests acrossFeatureGateTests,FeatureDisabledExceptionFilterTests,RequireFeatureEnabledAttributeTests, per-service disable tests onMonsterServiceandCleaningService,ProfileOverviewServicepre-validation tests,ProfileOverviewControllerexception-propagation tests).npm test— 627 passed (newdisabled-feature.guard.spec.tsincluding loadOnce-fails fallback; 403-with-disableKey case inerror.interceptor.spec.ts).npm run lintclean.npm run prettier-checkclean.dotnet format --verify-no-changesreports zero new violations from this PR.disable_mons=trueas admin. Confirm the Pokemon nav item disappears for both admin and non-admin sessions.POST /api/monsterswith curl as a non-admin → 403 with{ disableKey: "disable_mons" }./pokemondirectly → guard redirects to/dashboardwith toast.