Merge pull request #1 from itk-dev/feature/sec-review-1

Fixed issues raised by security review

Author: tuj

.claude/settings.json

@@ -1,7 +1,6 @@
 {
   "permissions": {
     "allow": [
-      "WebFetch(domain:raw.githubusercontent.com)",
       "Bash(task *)"
     ]
   }

.github/workflows/security.yaml

@@ -0,0 +1,36 @@
+name: Security
+
+env:
+  COMPOSE_USER: runner
+
+on:
+  pull_request:
+    paths:
+      - "composer.json"
+      - "docker-compose.yml"
+      - ".github/workflows/security.yaml"
+  push:
+    branches:
+      - main
+      - develop
+    paths:
+      - "composer.json"
+      - "docker-compose.yml"
+      - ".github/workflows/security.yaml"
+  schedule:
+    # Weekly run picks up newly-published advisories against unchanged deps.
+    - cron: "17 6 * * 1"
+
+jobs:
+  composer-audit:
+    runs-on: ubuntu-latest
+    steps:
+      - uses: actions/checkout@v6
+
+      - name: Create docker network
+        run: |
+          docker network create frontend
+
+      - run: |
+          docker compose run --rm --no-deps phpfpm composer install
+          docker compose run --rm --no-deps phpfpm composer audit

README.md

@@ -346,8 +346,7 @@ class User extends AbstractITKDevEntity implements AnonymizationStatusInterface
 ```
 
 Each PII field gets `#[ITKDev\EntityBundle\Privacy\Attribute\Anonymize(strategy: Strategy::Redact)]`. Strategies:
-`NullValue`, `Redact`, `Hash`, `Pseudonymize` (sha1 with a `kernel.secret`-derived pepper). The bundle scans these at
-compile time and ships two console commands:
+`NullValue`, `Redact`, `Hash`, `Pseudonymize`. The bundle scans these at compile time and ships two console commands:
 
 - `bin/console privacy:anonymize <subjectUlid>` — right-to-erasure: scrubs PII on the subject's User row + every row
   that references them via a `ManyToOne(UserInterface)` association, plus rewrites the corresponding audit history.
@@ -358,6 +357,19 @@ compile time and ships two console commands:
 
 The mechanism is law-neutral; the same machinery applies to GDPR, CCPA, LGPD, PIPEDA, etc.
 
+### GDPR semantics of strategies
+
+Not every strategy produces anonymous data. Pick deliberately:
+
+- `NullValue`, `Redact`, `Hash` — **anonymization.** Output is unlinkable to the source value and to other rows
+  scrubbed with the same strategy (`Hash` returns a fresh `random_bytes(32)`-derived token per call). Once applied, the
+  resulting column is no longer personal data.
+- `Pseudonymize` — **pseudonymization.** Output is a deterministic short token derived from the cleartext and
+  `kernel.secret`, so rows that shared a value still collide post-scrubbing. Use this when you need to preserve
+  referential equality (e.g. correlating activity across tables) without retaining the cleartext. Under GDPR
+  Recital 26 the result is **still personal data** — apply the same access controls as you would to the cleartext, and
+  do not export it as "anonymized."
+
 ## Configuration
 
 Everything is optional — see the reference table below for defaults.

SECURITY.md

@@ -0,0 +1,36 @@
+# Security policy
+
+## Supported versions
+
+Security fixes are issued for the latest minor release on the `main` branch.
+Older versions are not maintained — upgrade to receive fixes.
+
+## Reporting a vulnerability
+
+Please **do not** open a public GitHub issue for security problems.
+
+Report privately via GitHub's security advisory form:
+<https://github.com/itk-dev/entity-bundle/security/advisories/new>
+
+Include:
+
+- A description of the issue and its impact.
+- Steps to reproduce (a minimal failing case is ideal).
+- Affected versions, if known.
+- Your preferred contact for follow-up.
+
+## Scope
+
+This bundle handles personally identifiable data through its anonymization
+and audit features. Reports that materially affect the confidentiality or
+integrity of that data are in scope, including (non-exhaustive):
+
+- Anonymization strategies that fail to anonymize as documented.
+- Audit log entries that retain personal data after scrubbing.
+- SQL filter bypasses that expose soft-deleted or archived rows.
+- Dependency vulnerabilities surfaced by the project's `composer audit` job.
+
+Misuse by a host application (e.g. passing untrusted YAML into bundle
+configuration, exposing the privacy console commands to unauthenticated
+users) is out of scope; the bundle treats its configuration and CLI
+invocations as trusted.

src/Privacy/Strategy.php

@@ -4,6 +4,17 @@
 
 namespace ITKDev\EntityBundle\Privacy;
 
+/**
+ * GDPR semantics of each strategy:
+ *
+ * - NullValue, Redact, Hash → anonymization. Output is unlinkable to the
+ *   source value and to other rows scrubbed with the same strategy. Once
+ *   applied, the resulting column is no longer personal data.
+ * - Pseudonymize → pseudonymization. Output is deterministic in the source
+ *   value and kernel.secret; rows that shared a cleartext value still
+ *   collide post-scrubbing. The result remains personal data under GDPR
+ *   Recital 26 — treat with the same access controls as the cleartext.
+ */
 enum Strategy: string
 {
     case NullValue = 'null';

src/Privacy/StrategyApplier.php

@@ -19,7 +19,11 @@ public function apply(Strategy $strategy, mixed $value, ?string $replacement = n
         return match ($strategy) {
             Strategy::NullValue => null,
             Strategy::Redact => $replacement ?? '[REDACTED]',
-            Strategy::Hash => null === $value ? null : hash('sha256', (string) $value.$this->pepper),
+            // Per-call random output — unlinkable to the source value and to any other
+            // row anonymized with the same strategy. Pepper is intentionally unused here
+            // so the result cannot be recomputed from the cleartext even by an operator
+            // who knows kernel.secret.
+            Strategy::Hash => null === $value ? null : bin2hex(random_bytes(32)),
             Strategy::Pseudonymize => null === $value
                 ? null
                 : 'user_'.substr(hash('sha256', (string) $value.$this->pepper), 0, 12),

tests/Unit/Privacy/StrategyApplierTest.php

@@ -33,18 +33,32 @@ public function testRedactWithReplacement(): void
         self::assertSame('[GONE]', $this->applier->apply(Strategy::Redact, 'value', '[GONE]'));
     }
 
-    public function testHashOfNonNullValue(): void
+    public function testHashOfNonNullValueIsHex(): void
     {
-        self::assertSame(hash('sha256', 'value'.'test-pepper'), $this->applier->apply(Strategy::Hash, 'value'));
+        $result = $this->applier->apply(Strategy::Hash, 'value');
+
+        self::assertIsString($result);
+        self::assertMatchesRegularExpression('/^[0-9a-f]{64}$/', $result);
     }
 
     public function testHashOfNullReturnsNull(): void
     {
         self::assertNull($this->applier->apply(Strategy::Hash, null));
     }
 
-    public function testHashIsPepperDependent(): void
+    public function testHashIsNonDeterministic(): void
+    {
+        $a = $this->applier->apply(Strategy::Hash, 'alice@example.com');
+        $b = $this->applier->apply(Strategy::Hash, 'alice@example.com');
+
+        self::assertNotSame($a, $b);
+    }
+
+    public function testHashIsUnlinkableToSource(): void
     {
+        // Same source value across two different appliers (different peppers)
+        // must still produce unrelated outputs — Hash does not depend on the
+        // pepper, so determinism cannot leak via shared deployment secrets.
         $other = new StrategyApplier('different-pepper');
 
         self::assertNotSame(