a5a8ecdb20
Import initial de la documentation : statut, roadmap, plans V1/V2, specs verticales (creche, sport), audits securite, plan de test, analyse concurrentielle et maquettes de design. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
59 lines
7.4 KiB
Markdown
59 lines
7.4 KiB
Markdown
# Audit manager-service — incohérences & pistes d'amélioration
|
|
|
|
Audit du 2026-07-13/14. Trois axes analysés : Controllers/Security, Services/Data, DTOs/Config/Tests.
|
|
|
|
## Déjà corrigé (2026-07-14)
|
|
|
|
- **Clé API sur-privilégiée** — `ApiKeyAuthenticationHandler` ne donne plus le claim `ContentEditor` aux clés API (visiteurs). Seul le JWT peut écrire du contenu.
|
|
- **IDOR `UserController`** — `GetDetail`/`UpdateUser`/`DeleteUser` filtrent maintenant par `InstanceId` du claim (sauf SuperAdmin) ; `CreateUser` force l'`instanceId` de l'appelant ; garde-fou de rôle sur update/delete (impossible d'agir sur un rôle supérieur).
|
|
- **IDOR `DeviceController`** — `Get` filtre par instance, `GetDetail`/`Create` ne sont plus `AllowAnonymous`, `Update`/`UpdateMainInfos`/`Delete` vérifient l'appartenance à l'instance, changement d'instance interdit pour un non-SuperAdmin. NRE potentielle sur `applicationInstance` corrigée.
|
|
- **IDOR `AiController`/`StatsController`** — `instanceId` (query/body) comparé au claim de l'appelant (sauf SuperAdmin) avant tout traitement (évite la consommation du quota IA ou la lecture de stats d'une autre instance).
|
|
|
|
**Non traité volontairement** : `ConfigurationController` (lecture par `instanceId` via policy `AppReadAccess`) — probablement voulu car c'est du contenu public affiché aux visiteurs (musée), pas une donnée sensible. À confirmer avant de restreindre.
|
|
|
|
## Critique — non traité, priorité suivante
|
|
|
|
- **Secrets committés en clair dans Git** : JWT signing key, `SecuritySettings.Secret`, clés API OpenWeather/Gemini, connection strings Postgres/Mongo prod, mot de passe MQTT, token bot Telegram (`appsettings*.json`, `Deployment/.env`), pepper scrypt en dur (`ProfileLogic.cs`). Le dossier `RELEASE/` republie d'anciens secrets. Seul `firebase-adminsdk.json` est correctement ignoré.
|
|
→ Rotation de tous les secrets, passage en variables d'environnement/secrets Docker, purge de l'historique Git (`git filter-repo`), suppression de `RELEASE/`.
|
|
|
|
## Haute
|
|
|
|
- `Startup.cs` : `EnableSensitiveDataLogging()` + `LogTo(Console, Information)` actifs en permanence (toutes valeurs SQL loggées en prod) → conditionner à `IsDevelopment()`.
|
|
- Services Mongo legacy (`SectionDatabaseService`, `Instance/Configuration/Device/Resource/UserDatabaseService`) : `new MongoClient(...)` dans le constructeur en scope `AddScoped` → un pool de connexions par requête. À passer en singleton. Couche 100% synchrone/bloquante.
|
|
- `MyInfoMateDbContext` : conversions JSONB (`List<TranslationDTO>`) sans `ValueComparer` → une mutation in-place (ex. `AgendaSyncService.SetTranslation`) n'est pas détectée par EF ni persistée.
|
|
- `AssistantService.cs` : `new HttpClient()` manuel en boucle (risque d'épuisement de sockets) au lieu d'`IHttpClientFactory` ; duplication de la logique de fetch d'agenda déjà présente dans `AgendaSyncService` ; N+1 sur `Resources`.
|
|
|
|
## Moyenne
|
|
|
|
- JWT : `RequireExpirationTime = false` alors que `ValidateLifetime = true` (tokens sans `exp` acceptés, non révocables) ; `ValidateIssuer`/`ValidateAudience = false` alors que configurés (config morte).
|
|
- Clés API stockées en clair en plus du hash (`ApiKeyAuthenticationHandler.cs:40`, comparaison non constant-time).
|
|
- Bootstrap de clé API par pincode en `AllowAnonymous` sans rate limiting (`InstanceController.GetAppKeyByPin`/`GetInstanceByPinCode`) → brute-forçable.
|
|
- Exceptions internes exposées dans les réponses (`ex.Message` en 500, `AuthenticationController` sérialise l'objet exception entier) → middleware d'erreur global à généraliser.
|
|
- Backdoor `#if DEBUG` dans `AuthenticationController.Login` — retirer ou conditionner explicitement.
|
|
- Package `Microsoft.AspNetCore.Authentication.JwtBearer` v2.1.30 (ère .NET Core 2.1) sur une app .NET 8.
|
|
- Double `SaveChangesAsync` dans l'audit (`MyInfoMateDbContext.cs:67-74`) — 2e save no-op, `result` gonflé.
|
|
→ **À revérifier** : un bug voisin a été corrigé le 2026-08-06 — `BuildAuditEntries()` ajoutait un `AuditLog` au contexte *pendant* l'énumération du `ChangeTracker`, ce qui levait `InvalidOperationException: Collection was modified` sur **toute écriture d'entité auditée** (Section, Resource, Configuration, Device, User, Instance). Invisible jusque-là parce que la suite de tests ne compilait plus. Vérifier si ce point est devenu caduc.
|
|
- `SectionFactory` : ~700 lignes de mappings dupliqués (extraire `MapBase`), `dto.dateCreation.Value`/`.order.Value` sans garde (`InvalidOperationException`), double (dé)sérialisation System.Text.Json → Newtonsoft.
|
|
- `RemoteEventAgendaDTO.ParseDate` : fallback `DateTime.TryParse` sans `InvariantCulture` (ambiguïté JJ/MM selon locale serveur) ; `date_hour` jamais recombiné à la date ; `FalseToNullConverter` casse si l'API PHP renvoie `[]` au lieu de `false`. **Zéro test** sur ce code, pourtant le plus risqué du repo (parsing JSON externe non maîtrisé).
|
|
- `AppSettingsProvider` : état statique mutable alimenté depuis le constructeur de `Startup`, non thread-safe → passer à `IOptions<T>`.
|
|
- Tests : provider EF InMemory alors que le projet dépend de NetTopologySuite + JSONB → les requêtes spatiales/traductions Npgsql ne sont jamais réellement validées.
|
|
- Duplication massive du pattern try/catch dans les controllers (candidat à un filtre d'exception global).
|
|
- Absence quasi générale d'`AsNoTracking()` en lecture ; pas de `CancellationToken` sur les jobs Hangfire.
|
|
- `IHexIdGeneratorService` jamais enregistré en DI → `new HexIdGeneratorService()` instancié partout ; `System.Random` non thread-safe/non crypto pour la génération d'IDs (collisions possibles).
|
|
|
|
## Basse
|
|
|
|
- Conventions REST hétérogènes : id dans le body au lieu de la route (`PUT`/`DELETE`), `DELETE` renvoyant `202`+string au lieu de `204`, mélange `ObjectResult`/`IActionResult`, `SaveChanges()` sync dans des méthodes `async`.
|
|
- `PasswordUtils` : `System.Random` statique partagé pour générer codes/pincodes (prévisibles), MD5 obsolète, `RNGCryptoServiceProvider`/`MD5CryptoServiceProvider` dépréciés.
|
|
- Code mort : `ExternalService` (MqttClientService jamais utilisé, sous-système MQTT commenté dans Startup), policy CORS `AllowAll` définie mais jamais appliquée, `LanguageInit` statique mais injecté en DI.
|
|
- `ImageHelper.ResizeImage` retourne `dynamic`, objets GDI (`Graphics`/`Bitmap`/`Font`) non disposés, fragile sur Linux (`System.Drawing.Common`).
|
|
→ **Confirmé le 2026-08-07** : c'est du **code mort**. Le seul appelant est `ResourceController.Upload`, l'endpoint legacy base64 que manager-app n'utilise plus depuis la bascule Firebase. Et `System.Drawing.Common` ne fonctionne pas sur Linux depuis .NET 6 sans libgdiplus — l'image finale étant `aspnet:8.0` Linux, ce code lèverait une exception s'il était appelé. À supprimer ; le redimensionnement passe côté client (`v2/media-storage-plan.md`).
|
|
- CORS : domaines autorisés codés en dur dans `Configure` au lieu de venir de la config.
|
|
- Ordre middleware : `UseCors` placé après `UseAuthentication`/`UseAuthorization` — ordre recommandé : Routing → CORS → Authentication → Authorization.
|
|
|
|
## Notes complémentaires
|
|
|
|
- `WeatherSyncService.cs` : catch qui écrit sur `Console.WriteLine` au lieu d'`ILogger`.
|
|
- `AgendaSyncService.cs` : double ajout redondant (`section.EventAgendas.Add` + `db.EventAgendas.Add`) ; incohérence de pattern de scope Hangfire entre `AgendaSyncService` et `WeatherSyncService`.
|
|
- `SectionFactory.ToDTO` : sentinelle `StartDate?.Year > 1000` pour détecter une date "nulle" — fragile.
|